RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Roger Riggs]

Hi Pavel,

Many good improvements

WebSocket:

  - CloseReason.of; line 1187:  the new constructor of(ByteBuffer) is 
using IllegalArgumentException inappropriately
    (And other constructors too)
    Since the data some off the wire; it is not really the callers fault 
if it is too short or too long.
    There isn't really a good way to handle that case; throwing an 
exception to the caller isn't helpful
    It makes more sense to fix/hack up an approximation for the close 
reason.
    Perhaps convert it to a protocol error or truncate the data.

- Remove the use of Optional for CloseReason

WSMessageConsumer:

  - onClose: I agree that the Optional is a nuisance and using a empty 
message is easy to understand and use.

WSProtocolException:

  - Pre-existing: It looks a bit odd to feature the section number in 
the Exception;
     I think the detail message should be primary and provide the 
section information
     as secondary "(section x.y.x)". It is reasonably safe to assume that
     section numbers are relatively stable but someday may need a 
version number qualifier.

WSCloseCode: - I think there's too much ceremony around this 
implementation interface/class, etc;

   Its ok as is, but I would remove the interface and just keep the enum.
   The interface pattern is only needed for extensiblity of a public API.

Thanks, Roger



On 5/31/2016 1:25 PM, Pavel Rappo wrote:
> Hi,
>
> Could you please review the following change for JDK-8156742?
>
> http://cr.openjdk.java.net/~prappo/8156742/webrev.01/
>
> This change addresses the first group of WebSocket API refinements and
> enhancements from [1].
>
> 1. Change method `Builder#connectTimeout(long, TimeUnit)` to
> `Builder#connectTimeout(Duration)`
>
> Make use of convenience introduced with java.time API. The builder is not a
> performance-critical place, so there's no harm in constructing an object of
> `java.time.Duration` each time it's needed. Moreover, since 9, there's a bridge
> between TimeUnit and Duration: java.util.concurrent.TimeUnit.toChronoUnit
>
> 2. Change method `long WebSocket#request(long)` to `void WebSocket#request(long)`
>
> Otherwise a detail of implementation becomes a part of the spec. In this case
> it's not desirable, since we'll have to specify the behaviour in the corner case
> (long wrap) and force future implementations to maintain this abstraction.
>
> 3. Remove method `WebSocket#sendBinary(byte[], boolean)`
>
> This method provides not enough convenience to justify its existence.
>
> 4. Change type `CloseCode` for `CloseReason` that aggregates both status code
> and close reason.
>
> Current `Listener.onClose` looks ugly. It hides the otherwise explicit to all
> WebSocket users knowledge that 'reason' string can't go without the
> 'status code', i.e.:
>
> (statusCode reason?)?
>
> CloseReason types fuses both entities into a single type. As a bonus all
> knowledge about status code and reason string formats is now bound to a single
> place.
>
> 5. Specify `WebSocket#sendClose` idempotency
>
> Not producing IllegalStateException upon an attempt to close an already closed
> WebSocket seems to be a user-friendly solution. It's already an established practice
> in the JDK, e.g. java.nio.channels.SocketChannel.shutdownOutput
>
> 6. A number of miscellaneous editorial changes, missing copyright headers, tests.
>
> Thanks,
> -Pavel
>
> --------------------------------------------------------------------------------
> [1] https://bugs.openjdk.java.net/browse/JDK-8155621
>