RFR 8147505: Clarification of BaseStream.onClose() behavior

[Paul Sandoz]

Hi Tagir,

StreamCloseTest.java
—

 181         try(Stream<Integer> s = countTo(100).stream()) {
 182             s.map(x -> x).forEach(i -> {});
 183             checkISE(() -> s.onClose(() -> fail("2")));
 184         }

We don’t need this one, it’s redundant. The other performing the s.close() is i think useful because i don’t recall it being tested in other places like this, plus it also tests the idempotent behaviour, perhaps worth a comment on this in the test.

Otherwise, +1

Again, i will handle in bulk with your other fixes.

Many thanks,
Paul.

> On 23 Jan 2016, at 11:38, Tagir F. Valeev <amaembo at gmail.com> wrote:
> 
> Hello!
> 
> Here's a webrev:
> http://cr.openjdk.java.net/~tvaleev/webrev/8147505/r1/
> 
> I just added a check in AbstractPipeline.onClose and the corresponding
> unit-test. To me it seems that no documentation corrections are
> necessary, because "onClose" is already documented that it's
> an intermediate operation:
> 
> http://download.java.net/jdk9/docs/api/java/util/stream/BaseStream.html#onClose-java.lang.Runnable-
> 
> Also it's already documented:
> 
>> A stream should be operated on (invoking an intermediate or terminal
>> stream operation) only once. This rules out, for example, "forked"
>> streams, where the same source feeds two or more pipelines, or
>> multiple traversals of the same stream. A stream implementation may
>> throw IllegalStateException if it detects that the stream is being
>> reused.
> 
> http://download.java.net/jdk9/docs/api/java/util/stream/Stream.html
> 
> So this already implies that invoking intermediate operation "onClose"
> after stream terminal operation is not allowed and may throw
> IllegalStateException.
> 
> Please review!
> 
> With best regards,
> Tagir Valeev.
> 
> TFV> Seems that my original letter sounds confusing due to my bad English.
> TFV> Every occurrence of "idempotent" should be read as "not idempotent".
> TFV> Sorry for this!
> 
> PS>> We did not bother to throw ISEs from parallel/sequence/onClose
> PS>> primarily because they return “this” and they are kind of
> PS>> harmless, but the undefined behaviour you point out could be shut
> PS>> down without much concern for backwards compatibility. We can
> PS>> modify them to check whether they have been linked or consumed
> PS>> (while not updating the previous stage).
> 
> PS>> Wanna log an issue?
> 
> TFV> Sure:
> TFV> https://bugs.openjdk.java.net/browse/JDK-8147505
> 
> TFV> Thanks,
> TFV> Tagir Valeev.
> 
> PS>> Thanks,
> PS>> Paul.
> 
> 
>>>> On 15 Jan 2016, at 05:35, Tagir F. Valeev <amaembo at gmail.com> wrote:
>>>> 
>>>> Hello!
>>>> 
>>>> Current documentation for BaseStream.onClose() does not specify
>>>> explicitly how the stream would behave if additional close handlers
>>>> are registered after the stream is consumed or even closed. The
>>>> implementation actually allows registering the additional close
>>>> handlers after consumption or closure and close() method is
>>>> idempotent: newly registered handlers are perfectly called:
>>>> 
>>>> Stream<String> s = Stream.of("content");
>>>> s = s.onClose(() -> System.out.println("A"));
>>>> s.forEach(System.out::println);
>>>> s = s.onClose(() -> System.out.println("B"));
>>>> s.close(); // prints A and B
>>>> s = s.onClose(() -> System.out.println("C"));
>>>> s.close(); // prints C
>>>> 
>>>> (removing "s =" produces the same result).
>>>> 
>>>> I think if such behavior is intended, it should be explicitly noted in
>>>> the specification, as third-party implementations of BaseStream
>>>> interface should provide consistent behavior. Or at least it should be
>>>> noted that some BaseStream implementations may have idempotent close()
>>>> method, so the derived AutoCloseable objects (which own the BaseStream
>>>> object) should be aware about this behavior and provide idempotent
>>>> close() method as well. Without knowing this one may write:
>>>> 
>>>> class MyObject implements AutoCloseable {
>>>> private BaseStream<?, ?> s;
>>>> 
>>>> public MyObject(BaseStream<?, ?> s) {
>>>>   this.s = s;
>>>> }
>>>> 
>>>> @Override
>>>> public void close() throws Exception {
>>>>   if(s != null) {
>>>>     s.close();
>>>>     s = null;
>>>>   }
>>>> }
>>>> }
>>>> 
>>>> Such code closes the stream only once, so newly registered handlers
>>>> will not be called if MyObject::close is called the second time.
>>>> 
>>>> However, to my opinion the current behavior is weird and should be
>>>> changed in order not to allow registering new close handles (throwing
>>>> IllegalStateException) when the stream is already closed, or even
>>>> better when the stream is linked/consumed. As far as I know currently
>>>> in JDK close handlers are registered only for non-consumed stream, so
>>>> such change would not break existing JDK code (though may break some
>>>> strange third party code). It should be noted that AutoCloseable
>>>> interface discourages idempotent close() methods (though not forbids
>>>> them).
>>>> 
>>>> What do you think?
>>>> 
>>>> With best regards,
>>>> Tagir Valeev.
>>>> 
>