RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Felix Yang]

Hi Pavel,

     one comment:

http://cr.openjdk.java.net/~prappo/8156742/webrev.01/src/java.httpclient/share/classes/java/net/http/WebSocket.java.udiff.html

The CloseReason is constructed with read only Buffer as following:

+ private CloseReason(WSCloseCode code, String description, ByteBuffer 
data) {
+ int statusCode = code.getStatusCode();
+ if (statusCode < 1000 || statusCode > 4999) {
+ throw new IllegalArgumentException("Out of range: " + statusCode);
+ }
+ if (statusCode == 1004 || statusCode == 1005 || statusCode == 1006
+ || statusCode == 1015) {
+ throw new IllegalArgumentException("Reserved: " + statusCode);
+ }
+ ByteBuffer copiedData = ByteBuffer.allocate(data.remaining());
+ copiedData.put(data).flip();
+ data.flip();
+ this.code = code;
+ this.description = description;
+ this.data = copiedData.asReadOnlyBuffer();
          }

If a WebSocket tries to send it, it will fail immediately:
Caused by java.nio.ReadOnlyBufferException
     at 
java.nio.HeapByteBufferR.putLong(java.base at 9-internal/HeapByteBufferR.java:475)
     at 
java.net.http.WSFrame$Masker.loop(java.httpclient at 9-internal/WSFrame.java:158)
     at 
java.net.http.WSFrame$Masker.applyMask(java.httpclient at 9-internal/WSFrame.java:132)
     at 
java.net.http.WSMessageSender$FrameBuilderVisitor.maskAndRewind(java.httpclient at 9-internal/WSMessageSender.java:185)
     at 
java.net.http.WSMessageSender$FrameBuilderVisitor.visit(java.httpclient at 9-internal/WSMessageSender.java:174)
     at 
java.net.http.WSOutgoingMessage$Close.accept(java.httpclient at 9-internal/WSOutgoingMessage.java:156)
     at 
java.net.http.WSMessageSender.sendNow(java.httpclient at 9-internal/WSMessageSender.java:90)
     at 
java.net.http.WSMessageSender.trySendFully(java.httpclient at 9-internal/WSMessageSender.java:80)
     at 
java.net.http.WSTransmitter.handleSignal(java.httpclient at 9-internal/WSTransmitter.java:127)
     at 
java.net.http.WSSignalHandler.lambda$new$1(java.httpclient at 9-internal/WSSignalHandler.java:80)
     at 
java.util.concurrent.ThreadPoolExecutor.runWorker(java.base at 9-internal/ThreadPoolExecutor.java:1158)
     at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base at 9-internal/ThreadPoolExecutor.java:632)
     ... 1 more

-Felix
On 2016/6/1 1:25, 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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160601/000a8d49/attachment.html>