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