Hi, ah, sorry, seems I did not take a close enough look myself until now.
I wanted to be sure what the state was before and how it should be. See the "reproducer" project at https://github.com/centic9/poi-reproduce and the resulting summary: 3.17 4.0.0 4.0.1 4.1.0 4.1.1 4.1.2 5.0.0 5.1.0 5.2.0 5.2.1 5.2.2 5.2.3 5.2.4 trunk-2023-10-26 Goal Workbook-Stream closed closed leaked closed closed closed closed closed closed closed closed closed closed leaked closed Workbook-File fails, closed closed closed closed closed closed closed closed closed closed closed closed closed closed closed Workbook-String fails, closed closed closed closed closed closed closed closed closed closed closed closed closed closed closed OPCPacakge-Stream closed closed leaked closed closed closed closed closed closed closed closed closed closed leaked closed OPCPackage-File closed closed closed closed closed closed closed closed closed closed closed closed closed closed closed OPCPackage-String closed closed closed closed closed closed closed closed closed closed closed closed closed closed closed Broken-file - Stream leaked closed closed leaked leaked leaked leaked leaked leaked leaked leaked leaked closed leaked closed Broken-file - File leaked closed closed leaked leaked leaked leaked leaked leaked leaked leaked leaked leaked leaked closed Broken-file - String leaked closed closed leaked leaked leaked leaked leaked leaked leaked leaked leaked leaked leaked closed This indicates the following to me: * The bug-report about a "regression with closing streams in 5.2.4" seems to be incorrect. We closed provided InputStreams since a long time! * => So I do not think we should change close-behavior, we rather should revert the recent changes in trunk to restore the state of 5.2.4 for now * We can look at the special case of a file that is not a proper Zip-File, but this is a minor thing not relevant to the release Thanks... Dominik. On Wed, Oct 25, 2023 at 6:49 PM PJ Fanning <fannin...@yahoo.com.invalid> wrote: > I cannot reproduce the leaked file handle. This test case does not leak > files. I tested with `lsof -f`. I had a file handle until I closed the > xssfWorkbook but not after. > > File file = ... > XSSFWorkbook xssfWorkbook = new XSSFWorkbook(file); > // some Thread.sleep(...) here - use `lsof- f` during this sleep > xssfWorkbook.close(); > // some Thread.sleep(...) here - use `lsof- f` during this 2nd sleep > > Tested on a Mac with latest POI code in svn trunk. > > > > > > On Wednesday 25 October 2023 at 12:40:12 IST, PJ Fanning > <fannin...@yahoo.com.invalid> wrote: > > > > > > I will try to set up some debugging of the File path. I am not convinced > that the ZipPackage InputStream changes affect the behaviour when you > provide a File instead. The code handles Files and InputStreams differently. > > > > > > On Wednesday 25 October 2023 at 07:24:52 IST, Dominik Stadler < > dominik.stad...@gmx.at> wrote: > > > > > > Hi, > > We now leave open file-handles whenever the constructor for "File" is used > in ZipPackage. Not sure if the background Java support for closing stale > file-handles will capture those or if we will see "too many open files" in > large environments. > > Can we only wrap the stream in NoCloseStream when it is actually opened via > an incoming InputStream and not when it is opened from a File? > > Thanks... Dominik. > > On Wed, Oct 25, 2023 at 12:38 AM PJ Fanning <fannin...@apache.org> wrote: > > > Are we in a position to proceed with a 5.2.5 RC1? > > > > > > On 2023/10/16 10:23:42 Tim Allison wrote: > > > This just bit us on Tika: > > > https://bz.apache.org/bugzilla/show_bug.cgi?id=67767 > > > > > > The fix is easy. I can patch it today. It would be great to get it > into > > > 5.2.5. I'm sorry that I didn't catch it during the earlier regression > > > tests...my fault. > > > > > > On Sun, Oct 15, 2023 at 4:34 PM Dominik Stadler < > dominik.stad...@gmx.at> > > > wrote: > > > > > > > Hi, > > > > > > > > I use file-leak-detector when running tests locally, this shows any > > > > left-over file-handles when running tests via the IDE. > > > > > > > > See https://github.com/jenkinsci/lib-file-leak-detector > > > > > > > > If you clone the latest version and build the tool locally via Maven, > > you > > > > can add something like the following in the run-config in the IDE: > > > > > > > > > > > > > > > -javaagent:/opt/file-leak-detector/target/file-leak-detector-1.17-SNAPSHOT-jar-with-dependencies.jar=http=0,strong,excludes=/opt/poi/file-leak-detector.exclude,dumpatshutdown > > > > > > > > Then you will get a dump of all leftover file-handles at the end. > > > > > > > > In this case, it only happens if we enter the special catch-block in > > > > ZipPackage#ZipPackage(File, PackageAccess). > > > > > > > > One of the broken files from fuzzing seems to trigger this, > > constructing > > > > the package succeeds then, however with a resulting unclosed > > file-handle. > > > > > > > > So it seems to be less wide-spread than I originally thought and may > > > > actually have been the case before as well, not sure any more now... > > > > > > > > Dominik. > > > > > > > > On Sun, Oct 15, 2023 at 9:34 PM PJ Fanning > <fannin...@yahoo.com.invalid > > > > > > > wrote: > > > > > > > > > Thanks Dominik for the feedback. The probable breaking change was: > > > > > > > > > > > > > > > > > https://svn.apache.org/viewvc/poi/trunk/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/ZipPackage.java?r1=1909467&r2=1909466&pathrev=1909467 > > > > > > > > > > > > > > > There have been some more changes, eg using java.nio.files.File to > > create > > > > > InputStreams instead of using FileInputStream. > > > > > > > > > > For this reason, I would prefer to use a test based approach to > > ensuring > > > > > the 'close' is called. Ideally, I would like to leave behind a > > regression > > > > > test. > > > > > > > > > > The ZipPackage change (r1909467) only affects the use of > > InputStreams and > > > > > does not affect the use of File inputs. The ZipPackage code is very > > > > > different on whether you start by providing an InputStream as > > opposed to > > > > a > > > > > java.io.File. In the latter case, we use a ZipFile class instance > to > > do > > > > > random access to the wrapped files. > > > > > > > > > > Could you provide me with some example of what needs debugging? For > > > > > instance, would something like this not work ok? > > > > > > > > > > File file = .... > > > > > try (XSSFWorkbook wb = new XSSFWorkbook(file)) { > > > > > ... use wb > > > > > } > > > > > > > > > > Is the sort of usage you are suggesting where we would fail to > close > > an > > > > > InputStream? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sunday 15 October 2023 at 19:37:33 IST, Dominik Stadler < > > > > > dominik.stad...@gmx.at> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > FYI, the current fix for not closing streams also does not close > any > > > > stream > > > > > opened via a "File" parameter, so we are introducing many unclosed > > > > > resources via this change at the moment. > > > > > > > > > > Can we solve this somehow differently? Maybe cleanly revert the > > change > > > > > which introduced it? > > > > > > > > > > Thanks... Dominik. > > > > > > > > > > > > > > > > > > > > On Mon, Oct 9, 2023 at 1:47 PM PJ Fanning <fannin...@apache.org> > > wrote: > > > > > > > > > > > HI everyone, > > > > > > > > > > > > I think the regression in issue 67579 [1] is serious to warrant a > > new > > > > POI > > > > > > release. With that in mind, could we ramp down on changes while > we > > > > decide > > > > > > if we want to release soon. > > > > > > > > > > > > I can do this one again unless someone else wants to do it. > > > > > > > > > > > > Regards, > > > > > > PJ > > > > > > > > > > > > [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=67579 > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org > > > > > > For additional commands, e-mail: dev-h...@poi.apache.org > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org > > > > > For additional commands, e-mail: dev-h...@poi.apache.org > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org > > For additional commands, e-mail: dev-h...@poi.apache.org > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org > For additional commands, e-mail: dev-h...@poi.apache.org > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@poi.apache.org > For additional commands, e-mail: dev-h...@poi.apache.org > >
