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 <amae...@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 <amae...@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.
>>>>>
>>
