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@9-internal/HeapByteBufferR.java:475)
at
java.net.http.WSFrame$Masker.loop(java.httpclient@9-internal/WSFrame.java:158)
at
java.net.http.WSFrame$Masker.applyMask(java.httpclient@9-internal/WSFrame.java:132)
at
java.net.http.WSMessageSender$FrameBuilderVisitor.maskAndRewind(java.httpclient@9-internal/WSMessageSender.java:185)
at
java.net.http.WSMessageSender$FrameBuilderVisitor.visit(java.httpclient@9-internal/WSMessageSender.java:174)
at
java.net.http.WSOutgoingMessage$Close.accept(java.httpclient@9-internal/WSOutgoingMessage.java:156)
at
java.net.http.WSMessageSender.sendNow(java.httpclient@9-internal/WSMessageSender.java:90)
at
java.net.http.WSMessageSender.trySendFully(java.httpclient@9-internal/WSMessageSender.java:80)
at
java.net.http.WSTransmitter.handleSignal(java.httpclient@9-internal/WSTransmitter.java:127)
at
java.net.http.WSSignalHandler.lambda$new$1(java.httpclient@9-internal/WSSignalHandler.java:80)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@9-internal/ThreadPoolExecutor.java:1158)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@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
