Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-27 Thread Alan Bateman
On 26/07/2019 20:01, Brian Burkhalter wrote: So updated: http://cr.openjdk.java.net/~bpb/8078891/webrev.02/ This version is a lot cleaner and looks okay to me. -Alan

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-27 Thread Alan Bateman
On 26/07/2019 20:37, Pavel Rappo wrote: For the record. If we change this as you suggested, the code will behave differently in the case of a single `null` element found in the midst of iteration (broken enumeration?). The stream won't be able to close after running into it. But this most likely

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Pavel Rappo
> On 26 Jul 2019, at 19:36, Alan Bateman wrote: > > > > In any case, the change looks okay but it might be simpler to restructure to > use: > > while (in != null) { > try { in.close() } catch (IOException e) { ... } > peekNextStream(); > } > > to simplify the error handling and

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Pavel Rappo
> On 26 Jul 2019, at 19:36, Alan Bateman wrote: > > The bug summary might be a big misleading as SequenceInputStream isn't thread > safe and close isn't atomic. The changes are really to attempt to close all > the remaining streams rather than bailing out when closing one of them fails. I'm

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Brian Burkhalter
> On Jul 26, 2019, at 11:36 AM, Alan Bateman wrote: > > On 26/07/2019 16:41, Brian Burkhalter wrote: >> : >> Please see the updated patch which switches the implementation to the >> amended version and incorporates the proposed change to the test: >> >>

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Alan Bateman
On 26/07/2019 16:41, Brian Burkhalter wrote: : Please see the updated patch which switches the implementation to the amended version and incorporates the proposed change to the test: http://cr.openjdk.java.net/~bpb/8078891/webrev.01/ The bug summary might be a big misleading as

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Daniel Fuchs
On 26/07/2019 17:41, Brian Burkhalter wrote: Please see the updated patch which switches the implementation to the amended version and incorporates the proposed change to the test: http://cr.openjdk.java.net/~bpb/8078891/webrev.01/ That looks good to me Brian. I had a look at the API

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Brian Burkhalter
> On Jul 26, 2019, at 4:14 AM, Pavel Rappo wrote: > > […] I prefer the amended version. As it > both does the job and preserves the original behavior when consuming data from > the current component stream. > > […] > > A nit on the accompanying test. Please consider this change: Please see

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-26 Thread Pavel Rappo
> On 25 Jul 2019, at 21:01, Brian Burkhalter > wrote: > This concern would be fixed by changing the proposed patch as > > --- a/src/java.base/share/classes/java/io/SequenceInputStream.java > +++ b/src/java.base/share/classes/java/io/SequenceInputStream.java > @@ -91,13 +91,10 @@ > *

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-25 Thread Brian Burkhalter
Hi Pavel, > On Jul 25, 2019, at 12:43 PM, Pavel Rappo wrote: > > Thanks a lot for picking up a bug I filled back in 2015. This looks like a > good > cleanup! Thanks for looking at it. > I'm a bit concerned though with the added `finally` block in L98. Could that > lead to a subtle behavioral

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-25 Thread Pavel Rappo
Brian, Thanks a lot for picking up a bug I filled back in 2015. This looks like a good cleanup! I'm a bit concerned though with the added `finally` block in L98. Could that lead to a subtle behavioral change for (an unlikely) case where a `read` method exhausts the current stream, then tries to

8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

2019-07-23 Thread Brian Burkhalter
https://bugs.openjdk.java.net/browse/JDK-8078891 http://cr.openjdk.java.net/~bpb/8078891/webrev.00/ Ensure that SequenceInputStream closes all component streams. Thanks, Brian