RFR JDK-8087113: Websocket API and implementation

[Roger Riggs]

Hi Pavel,

Initial comments, bottom up.

It would be helpful if the classnames/filenames reflected the 
participation in the WebSocket implementation
to keep them distinct from the HTTP 2.0 implementation in the same 
directory.
For example, Writer, Reader, etc.  perhaps a 'Ws' prefix would be 
sufficient.

This would be alleviated if websocket was in its in its own package. 
java.net.ws
(though I realize that may already have been settled, and I'm sure there 
are some dependencies
that would be nice to keep as package private).


Logging is sprinkled throughout the APIs of the implementation; it would 
be cleaner to have
a shared WS logging class with a static logger similar to the Http Log 
(or perhaps some sharing and reuse)
so the logging does not clutter the implementation interfaces.  It would 
rare that separate logger
instances are needed.

Writer.java:

- line 143:  keep track of the current/next buffer index of the first 
non-empty buffer; instead of replacing with NULL_BYTE_BUFFER;
  a combination of the buffers array index vs length and hasRemaining 
could eliminate the need to copy the buffers array
  and perform the initialization to determine the total length (though 
that is informative for logging)
   - not sure why you are using the 3 arg form of GatheringWriter since 
the byteBuffers array is exactly the length.

- line 181: the assert would be more useful inside the loop; catching an 
error when it occurs.
    Assert reason is more useful with a short descriptive text.
- line 184:   editorial, remove "from" from the logger

- line 228: can the Throwable be discarded?  Does it contain no information?


Reader.java:
  - line 137:  Why should the reader force a new task?  Should it be 
left to the subscriber to
     decide whether its processing is lightweight and can be done 
immediately or to re-submit to its own executor.


java.net.Utils:
  - line 231:  SATURATING_INCREMENT (and its uses) is wasted code; long 
is not going to roll over.

- line 236: NULL_BYTE_BUFFER isn't null - prehaps rename to 
EMPTY_BYTE_BUFFER

$.02, Roger




On 3/25/2016 12:21 PM, Pavel Rappo 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/
>