You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by PJ Fanning <fa...@apache.org> on 2023/10/09 11:47:37 UTC
POI 5.2.5 release
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-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by Dominik Stadler <do...@gmx.at>.
Hi,
According to my testing, ALL versions since 3-something did close
InputStream when you use XSSFWorkbook or OPCPackage, so this has been the
established behaviour for quite some time in Apache POI.
The change in ZipPackage in 66584 seems to have caused a close() even if no
exception is thrown, which is causing the problem reported in #67579.
So I think it best for now to revert both changes so we are back at what
5.2.3 did, not sure what 66584 actually fixed anyway.
Dominik.
On Thu, Oct 26, 2023 at 5:18 PM PJ Fanning <fa...@yahoo.com.invalid>
wrote:
> HSSFWorkbook(InputStream) closes its InputStream. So this is really a
> mess. So I guess the best is for XSSFWorkbook to also close
> its InputStream. We can document this and warn users that this is the
> behaviour and that they can work around it by using something like
> NoCloseInputStream.
>
>
> https://github.com/apache/poi/blob/f64524916d449d37350e0763e4f86edc239bbd34/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/internal/NoCloseInputStream.java
>
>
>
>
>
>
>
>
>
> On Thursday 26 October 2023 at 15:44:03 IST, PJ Fanning <
> fanningpj@yahoo.com> wrote:
>
>
>
>
>
> Can I clarify?
>
> You want https://bz.apache.org/bugzilla/show_bug.cgi?id=67579 reverted?
>
> If I interpret the poi-reproduce results correctly, there is a regression
> from 5.2.3 to 5.2.4 in the stream case.
>
> The behaviour might have changed in 5.2.2 to 5.2.3 migration but it still
> means that there is a regression in 5.2.3 to 5.2.4.
>
> Surely, we should not be closing input streams - they should be closed by
> the provider. We should only close input streams that our code creates
> internally.
>
> What if we call the prospective release 5.3.0 and keep the 67579 fix?
>
>
>
> On Thursday 26 October 2023 at 15:16:48 IST, Dominik Stadler <
> dominik.stadler@gmx.at> wrote:
>
>
>
>
>
> 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 <fa...@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
> > <fa...@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.stadler@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 <fa...@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.stadler@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
> > <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > > > For additional commands, e-mail: dev-help@poi.apache.org
> >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > For additional commands, e-mail: dev-help@poi.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@yahoo.com.INVALID>.
HSSFWorkbook(InputStream) closes its InputStream. So this is really a mess. So I guess the best is for XSSFWorkbook to also close its InputStream. We can document this and warn users that this is the behaviour and that they can work around it by using something like NoCloseInputStream.
https://github.com/apache/poi/blob/f64524916d449d37350e0763e4f86edc239bbd34/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/internal/NoCloseInputStream.java
On Thursday 26 October 2023 at 15:44:03 IST, PJ Fanning <fa...@yahoo.com> wrote:
Can I clarify?
You want https://bz.apache.org/bugzilla/show_bug.cgi?id=67579 reverted?
If I interpret the poi-reproduce results correctly, there is a regression from 5.2.3 to 5.2.4 in the stream case.
The behaviour might have changed in 5.2.2 to 5.2.3 migration but it still means that there is a regression in 5.2.3 to 5.2.4.
Surely, we should not be closing input streams - they should be closed by the provider. We should only close input streams that our code creates internally.
What if we call the prospective release 5.3.0 and keep the 67579 fix?
On Thursday 26 October 2023 at 15:16:48 IST, Dominik Stadler <do...@gmx.at> wrote:
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 <fa...@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
> <fa...@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.stadler@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 <fa...@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.stadler@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
> <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > > For additional commands, e-mail: dev-help@poi.apache.org
>
> > > > > >
> > > > > >
> > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > >
> > > > >
> > > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@yahoo.com.INVALID>.
Can I clarify?
You want https://bz.apache.org/bugzilla/show_bug.cgi?id=67579 reverted?
If I interpret the poi-reproduce results correctly, there is a regression from 5.2.3 to 5.2.4 in the stream case.
The behaviour might have changed in 5.2.2 to 5.2.3 migration but it still means that there is a regression in 5.2.3 to 5.2.4.
Surely, we should not be closing input streams - they should be closed by the provider. We should only close input streams that our code creates internally.
What if we call the prospective release 5.3.0 and keep the 67579 fix?
On Thursday 26 October 2023 at 15:16:48 IST, Dominik Stadler <do...@gmx.at> wrote:
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 <fa...@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
> <fa...@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.stadler@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 <fa...@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.stadler@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
> <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > > For additional commands, e-mail: dev-help@poi.apache.org
>
> > > > > >
> > > > > >
> > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > >
> > > > >
> > > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by Dominik Stadler <do...@gmx.at>.
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 <fa...@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
> <fa...@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.stadler@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 <fa...@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.stadler@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
> <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > > For additional commands, e-mail: dev-help@poi.apache.org
>
> > > > > >
> > > > > >
> > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > >
> > > > >
> > > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@yahoo.com.INVALID>.
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 <fa...@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 <do...@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 <fa...@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 <do...@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 <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > >
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > >
> > > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@yahoo.com.INVALID>.
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 <do...@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 <fa...@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 <do...@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 <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > >
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > >
> > > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by Dominik Stadler <do...@gmx.at>.
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 <fa...@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 <do...@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 <fanningpj@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > > >
> > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > >
> > > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@apache.org>.
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 <do...@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 <fa...@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > > For additional commands, e-mail: dev-help@poi.apache.org
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > > For additional commands, e-mail: dev-help@poi.apache.org
> > >
> > >
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by Tim Allison <ta...@apache.org>.
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 <do...@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 <fa...@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > > For additional commands, e-mail: dev-help@poi.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@yahoo.com.INVALID>.
Only a small fraction of the tests using try-with-resources. So it is just possible that there are some bad tests that don't close the resources explicitly.
On Sunday 15 October 2023 at 21:34:52 IST, Dominik Stadler <do...@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 <fa...@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by Dominik Stadler <do...@gmx.at>.
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 <fa...@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.stadler@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 <fa...@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-unsubscribe@poi.apache.org
> > For additional commands, e-mail: dev-help@poi.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
Re: POI 5.2.5 release
Posted by PJ Fanning <fa...@yahoo.com.INVALID>.
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 <do...@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 <fa...@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-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: POI 5.2.5 release
Posted by Dominik Stadler <do...@gmx.at>.
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 <fa...@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-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
Re: POI 5.2.5 release
Posted by Kamal Chandrashekar <sa...@gmail.com>.
pls enjoy code and wait for sometime. iam OOO.
On Mon, Oct 9, 2023, 5:17 PM PJ Fanning <fa...@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-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>