> On 6 May 2016, at 09:16, Felix Yang <felix.y...@oracle.com> wrote:
> 
> Hi Pavel,
>     several comments:
> 
> 1.  WebSocket.request(long n) is documented as "
> 
> @throws IllegalArgumentException if n < -1
> "
> 
> It looks meaningless to allow 0.

First of all, the way `request` is defined now is by no means meaningless.

     * @return resulting unfulfilled demand with this request taken into account
     */
    long request(long n);

Passing zero allows you, for instance, to get current demand without modifying 
it. 
But, this is going to be removed [1] (WebSocket API tasks, item 10).

Think of the ability to pass 0 as of a degree of robustness. That's a corner
case. It allows you to put some computed value, not necessarily a literal as it
does not require you to check the value for 0. As far as it is non-negative, no
harm is done.

If you're still not convinced, consider any of the following:

        "abc".substring(1, 1);
        collection.removeAll(Collections.emptyList());
        inputStream.read(new byte[0]);
        byteChannel.read(ByteBuffer.allocate(0));

> 2.  Some concern on the way of handling close. Consider following scenario.
> 
>       • obtain a ws connection
>       • message communications balabala...
>       • initialize closing handshake on either client/server peer
>       • if the client side didn't request adequate size or not requested at 
> all, it actually doesn't know connection is closed. "onClose" will never be 
> invoked, because close message is at the end of "queue".

True. That's the cost of providing reactive back-pressure. Otherwise we would
have to read off the socket in the background looking for the Close message to
appear.
 
>       • it looks to me that:
>               • the closing handshake can be blocked by UN-requested 
> messages. 
>               • a websocket can be in a state that isClosed() return false 
> but it can't send any message. Note that the state is not transient.

WebSocket is not necessarily closed after you send a Close message:

    /**
     * Tells whether the {@code WebSocket} is closed.
     *
     * <p> A {@code WebSocket} deemed closed when either the underlying socket
     * is closed or the closing handshake is completed.
     *
     * @return {@code true} if the {@code WebSocket} is closed,
     *         {@code false} otherwise
     */
    boolean isClosed();

> Filed a bug https://bugs.openjdk.java.net/browse/JDK-8156191. It looks 
> current design treats CloseMessage in similar way with other messages. I'm 
> not sure if it is worth something like WSClosingHandshake.
> 3. I'm concerning the bug in #2 is side-effect of reactive mode. May I ask 
> what is the benefit of reactive mode. WebSocket.request(long n) seem to be 
> good in first glance but:
>       • It looks end users need to calculate how many messages are expected. 
> The another question comes 
>               • what messages will be taken into account. How about PING/PONG 
> or else?
>       • We may use request( a big value up to Long.MAX). But, in this case, 
> why we need it?  This looks simple but a bit problematic.

You can read on benefits of reactive mode here [2, 3]. And feel free to read the
API thread [4] as well.

--------------------------------------------------------------------------------
[1] https://bugs.openjdk.java.net/browse/JDK-8155621
[2] http://www.reactive-streams.org
[3] https://github.com/reactive-streams/reactive-streams-jvm/
[4] http://mail.openjdk.java.net/pipermail/net-dev/2015-August/009077.html
    http://mail.openjdk.java.net/pipermail/net-dev/2015-September/009079.html
    http://mail.openjdk.java.net/pipermail/net-dev/2015-October/009223.html


Reply via email to