RFR 8147505: Clarification of BaseStream.onClose() behavior

[Tagir F. Valeev]

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