Bug 64542 - FileBackedDataSource closing external FileChannel
Summary: FileBackedDataSource closing external FileChannel
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: POIFS (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-19 20:58 UTC by Arjohn Kampman
Modified: 2020-06-22 15:08 UTC (History)
0 users



Attachments
Patch for POIFS ticket 64542 (3.70 KB, patch)
2020-06-22 14:07 UTC, Arjohn Kampman
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arjohn Kampman 2020-06-19 20:58:43 UTC
I am creating POIFSFileSystem instances in various ways, depending on what I have available: a File, FileChannel or InputStream. I just found out that calling POIFSFileSystem.close() (via a try-with-resources) also closes the FileChannel, even when that channel is passed to it from an external source. Any subsequent operations on that channel (like calculating an MD5 hash) then run into a ClosedChannelException. IMHO POIFSFileSystem/FileBackedDataSource  should only close the FileChannel if it has created it itself. It would be great if you could fix this.
Comment 1 PJ Fanning 2020-06-19 21:29:53 UTC
Hi Arjohn,

I'm not sure this is an easy change and the code has been like this for a long time - so there are risks that people rely on the behaviour as it is.

You might be able to wrap your FileChannel with a wrapper that delegates all calls to the wrapped channel but the wrapper could have a no-op close().

If you feel strongly about this issue, maybe you could submit a patch. If so, I would recommend adding a new constructor to FileBackedDataSource that takes a flag that says not to close the FileChannel (or a set method).
Comment 2 Arjohn Kampman 2020-06-19 21:50:55 UTC
Hi PJ,

I have tried the wrapper approach, but unfortunately the close() method is declared "final" in superclass AbstractInterruptibleChannel.

Maybe a third constructor variant for FileChannel would be an option? E.g.: new POIFSFileSystem(FileChannel channel, boolean readOnly, boolean closeChannel)
Comment 3 PJ Fanning 2020-06-19 21:53:35 UTC
Yes - a third constructor - that allows the existing constructors to retain their existing behaviours - makes sense.
Comment 4 Arjohn Kampman 2020-06-19 21:55:38 UTC
Do you want me to supply a patch for this? Looks pretty simple to add.
Comment 5 PJ Fanning 2020-06-19 22:07:33 UTC
Feel free to submit a patch.
Comment 6 Arjohn Kampman 2020-06-22 14:07:02 UTC
Created attachment 37322 [details]
Patch for POIFS ticket 64542
Comment 7 PJ Fanning 2020-06-22 15:08:48 UTC
Thanks for the patch but could you add a unit test?