RFR 8147505: Clarification of BaseStream.onClose() behavior

[Tagir F. Valeev]

Hello!

Thanks for review! Updated webrev:
http://cr.openjdk.java.net/~tvaleev/webrev/8147505/r2/
Removed redundant test and added a comment.

With best regards,
Tagir Valeev.

PS> Hi Tagir,

PS> StreamCloseTest.java
PS> —

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

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

PS> Otherwise, +1

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

PS> Many thanks,
PS> 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.
>>>>> 
>>