RFR JDK-8087113: Websocket API and implementation

[Andrej Golovnin]

Hi Pavel,

I have only cosmetic comments. Please feel free to ignore anything you don’t like. ;-)

> Could you please review my change for JDK-8087113
> 
>   http://cr.openjdk.java.net/~prappo/8087113/webrev.03/

src/java.httpclient/share/classes/java/net/http/RawChannel.java

When you don’t plan to have any subclasses of RawChannel, then
please mark it as final.

43     private boolean closed;

I think this field should be volatile.


src/java.httpclient/share/classes/java/net/http/Utils.java

This class should be final. And please add a private empty constructor.

215                 .append(" rem=").append(b.remaining()).append("]").toString();

Please use ']' instead of "]”.

222                 .append("[len=").append(s.length()).append("]").toString();

Please use ']' instead of "]”.

229     final static LongBinaryOperator SATURATING_INCREMENT

It should be “static final” and not “final static”.

232     static final System.Logger logger = System.getLogger("java.net.http.WebSocket”);

logger should be in uppercase.

234     static final ByteBuffer NULL_BYTE_BUFFER = ByteBuffer.allocate(0);

Maybe EMPTY_BYTE_BUFFER is a better name. Without seeing the code
I would expect that NULL_BYTE_BUFFER is the same as null and not an empty buffer.


src/java.httpclient/share/classes/java/net/http/package-info.java

31  * WebSocket works is asynchronous mode only. The main types defined are:

Typo: "works is asynchronous” -> "works in asynchronous”.


src/java.httpclient/share/classes/java/net/http/BuffersSubscriber.java

393         (this.subscription = requireNonNull(subscription)).request(1);

Code like this is difficult to read and understand. Maybe you should rewrite it using two lines of code.


src/java.httpclient/share/classes/java/net/http/CharsetToolkit.java

Maybe the lines 36-45 should be converted to a JavaDocs comment.

52     static class Verifier {

The class should be final.

92         public final CoderResult encode(CharBuffer in, ByteBuffer out, boolean endOfInput)

The class Encoder is final. What is the purpose to mark this method as final? And I think
it should not be public.

132         public final CoderResult decode(ByteBuffer in, CharBuffer out, boolean endOfInput)

The class Decoder is final. What is the purpose to mark this method as final? And I think
it should not be public.


src/java.httpclient/share/classes/java/net/http/MessagePublisher.java

107             // message and and enqueueing the pair.

Typo: one “and” too much.

177                             throw new IllegalArgumentException(String.valueOf(n));

Could you please provide a better message for exception here?

192     private static class CheckingVisitor implements Visitor<Void, Void> {

This class can be final.

 264                 try {
 265                     encodedReason = StandardCharsets.UTF_8.newEncoder()
 266                             .encode(CharBuffer.wrap(close.reason));
 267                 } catch (CharacterCodingException e) {
 268                     throw new IllegalArgumentException(
 269                             "The closure reason is a malformed UTF-16 sequence", e);
 270                 }

I think it should be "a malformed UTF-8 sequence” as you use UTF-8 charset.

 287         private void checkSize(int size, int maxSize) {
 288             if (size > maxSize) {
 289                 throw new IllegalArgumentException
 290                         ("The message is too long: " + size);
 291             }
 292         }

Please add maxSize to the exception message too.


src/java.httpclient/share/classes/java/net/http/MessageSender.java

116                             throw new IllegalArgumentException(String.valueOf(n));

Could you please provide a better message for exception here?

340                     throw new IllegalArgumentException("Malformed UTF-16 characters", e);

I think the message of the exception should be "Malformed UTF-8 characters”.

388         (this.subscription = Objects.requireNonNull(subscription)).request(1);

Objects.requireNonNull is already imported using a static import. But again I would
rewrite it and use two lines of code as code like this is difficult to read and understand.


src/java.httpclient/share/classes/java/net/http/OpeningHandshake.java

  57     private final static SecureRandom srandom;
  58     private final static MessageDigest sha1;

It should be “private static final”.

96                 webSocketUri.getScheme().equals("ws") ? "http" : "https”;

Maybe you should use here equalsIgnoreCase().


src/java.httpclient/share/classes/java/net/http/Reader.java

46     private volatile Flow.Subscriber<? super Shared<ByteBuffer>> subscriber;

I would move this line after the line with the last final field (private final RawChannel channel).

55     private volatile boolean started, closed;

Please use separate declaration for every field.

93                     throw new IllegalStateException();

Please add a message to the exception.

192                             throw new IllegalArgumentException(String.valueOf(n));

Could you please provide a better message for exception here?


src/java.httpclient/share/classes/java/net/http/SequentialExecutor.java

Maybe the lines 9-12 should be converted to a JavaDocs comment.


src/java.httpclient/share/classes/java/net/http/Shared.java

Maybe the lines 33-43 should be converted to a JavaDocs comment.

 156             // We don't work with buffers of other types
 157             throw new IllegalArgumentException();

Please add a message to the exception. The same applies for the lines 171 and 184.


src/java.httpclient/share/classes/java/net/http/SignalHandler.java

Maybe the lines 33-46 should be converted to a JavaDocs comment.

 112             switch (s) {
 113             case RUNNING:
 114                 return RERUN;
 115             case DONE:
 116                 return RUNNING;
 117             case RERUN:
 118                 return RERUN;
 119             default:
 120                 throw new InternalError(String.valueOf(s));
 121             }

Please fix the indentation.

 120                 throw new InternalError(String.valueOf(s));

Could you please provide a better message for exception here?


src/java.httpclient/share/classes/java/net/http/WebSocket.java

Could you please add an #of-method to CloseCode which also takes
a description as parameter, e.g.:

public static CloseCode of(int code, String description)

1221             return Objects.hash(code);

I think it should be changed to return just the code.


src/java.httpclient/share/classes/java/net/http/WebSocketBuilderImpl.java

45     private final static Set<String> forbiddenHeadersLowerCased = Set.of(

It should be “private static final”. And forbiddenHeadersLowerCased should be in uppercase.

74         if (forbiddenHeadersLowerCased.contains(name.toLowerCase())) {

Please use name.toLowerCase(Locale.ROOT). Another solution is to use
a TreeSet with the String.CASE_INSENSITIVE_ORDER comparator.


src/java.httpclient/share/classes/java/net/http/WebSocketFrame.java

161         private final static class Masker64 extends Masker {

It should be “private static final”.

195         private final static class Masker32 extends Masker {

It should be “private static final”.

 244             if (value)
 245                 firstChar |= 0b10000000_00000000;
 246             else
 247                 firstChar &= ~0b10000000_00000000;

Please add curly braces. The same applies to the lines 256-259, 268-271, 280-283, 300-305.

 393         // A private buffer used to simplify multi-byte integers reading
 394         private final ByteBuffer accumulator = ByteBuffer.allocate(8);

The declaration of this field should be moved after the declaration of constants in the Reader class.


src/java.httpclient/share/classes/java/net/http/WebSocketHandshakeException.java

36     private transient HttpResponse response;

The field can be final.


src/java.httpclient/share/classes/java/net/http/WebSocketImpl.java

310                         throw new IllegalStateException();

Please add a message to the exception.

 359     private enum State {
 360         CONNECTED,
 361         CLOSED_REMOTELY,
 362         CLOSED_LOCALLY,
 363         CLOSED {
 364             @Override
 365             boolean isClosed() { return true; }
 366         },
 367         ABORTED {
 368             @Override
 369             boolean isClosed() { return true; }
 370         };
 371 
 372         boolean isClosed() { return false; }
 373     }

When you change the implementation of the isClosed()-method as follows:

boolean isClosed() { return this == CLOSED || this == ABORTED; }

then you can avoid anonymous inner sub-classes.


src/java.httpclient/share/classes/java/net/http/Writer.java

207         (this.subscription = requireNonNull(subscription)).request(1);

Code like this is difficult to read and understand. Maybe you should rewrite it using two lines of code.


test/java/net/httpclient/HandshakePhase.java

The copyright header is missed.


Best regards,
Andrej Golovnin