RFR JDK-8087113: Websocket API and implementation

[Felix Yang]

Hi Pavel,

     several comments:

1.  WebSocket.request(long n) is documented as "

@throws IllegalArgumentException if n < -1

"

It looks meaningless to allow 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".
  * it looks to me that:
      o the closing handshake can be blocked by UN-requested messages.
      o 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.

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


My $0.02,
Felix

On 2016/5/3 23:23, Pavel Rappo wrote:
> Hello,
>
> Here's an updated webrev with the latest implementation:
>
>     http://cr.openjdk.java.net/~prappo/8087113/webrev.04/
>
> If you're going to review it, please be advised that there is a number of known
> API and implementation issues yet to be resolved [1]. More API tests will be
> provided in the next couple of months.
>
> All applicable tests (1.1.1-10.1.1, 305 in total) from Autobahn Testsuite [2]
> are green.
>
> Thanks,
> -Pavel
>
> --------------------------------------------------------------------------------
> [1] https://bugs.openjdk.java.net/browse/JDK-8155621
> [2] http://autobahn.ws/testsuite/
>
>> On 25 Mar 2016, at 16:21, Pavel Rappo <pavel.rappo at oracle.com> wrote:
>>
>> Hi,
>>
>> Could you please review my change for JDK-8087113
>>
>>    http://cr.openjdk.java.net/~prappo/8087113/webrev.03/
>>
>> This webrev contains initial implementation and basic tests for WebSocket API.
>> Specification conformance & interoperability testing has been performed with
>> Autobahn Testsuite [1]. As for API tests, I will provide more of them later.
>>
>> Thanks,
>> -Pavel
>>
>> --------------------------------------------------------------------------------
>> [1] http://autobahn.ws/testsuite/
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160506/0fe0c0a6/attachment.html>