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