RFR JDK-8087113: Websocket API and implementation
Pavel Rappo
pavel.rappo at oracle.com
Mon Mar 28 20:19:43 UTC 2016
> On 26 Mar 2016, at 11:44, Andrej Golovnin <andrej.golovnin at gmail.com> wrote:
>
> 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.
Done.
> 215 .append(" rem=").append(b.remaining()).append("]").toString();
>
> Please use ']' instead of "]”.
>
> 222 .append("[len=").append(s.length()).append("]").toString();
>
> Please use ']' instead of "]”.
I believe since JEP 280 [1] is already here, we don't need to bother about this
kind of things any more (generally speaking).
> 229 final static LongBinaryOperator SATURATING_INCREMENT
>
> It should be “static final” and not “final static”.
Done.
> 232 static final System.Logger logger = System.getLogger("java.net.http.WebSocket”);
>
> logger should be in uppercase.
I have some doubts about this one. As I understand the rule is more about
constant values in a broad sense. E.g. primitive, String, maybe immutable Map/
Collection, etc.
System.Logger, represents more a behaviour class. It's not a value sort of
class. Consider java.lang.System#out/#err/#in
> 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.
Maybe you're right. I was referring to "Null Object" pattern rather than null
literal.
> 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”.
Done.
> 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.
Done.
> 177 throw new IllegalArgumentException(String.valueOf(n));
>
> Could you please provide a better message for exception here?
What would the purpose be? It's an internal implementation detail. If this
exception is thrown, it means it's a bug in the implementation. And I highly
doubt this error message would be of any help to an end-user. It's not something
they can influence/change (most likely).
What's important here is the stack-trace and te value. Maybe I will include more
state to be printed.
> 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 }
Done.
> I think it should be "a malformed UTF-8 sequence” as you use UTF-8 charset.
It's a bit tricky here. This piece of code verifies that a Close Reason attached
to the Close message can be encoded into UTF-8 bytes. But the Close Reason
itself is a java.lang.CharSequence and therefore is a UTF-16 representation. If
you have a look at java.nio.charset.CharsetEncoder#encode(java.nio.CharBuffer)
it states clearly:
* @throws MalformedInputException
* If the character sequence starting at the input buffer's current
* position is not a legal sixteen-bit Unicode sequence...
> 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.
Done.
> 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);
(see comments above)
> 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.
Done.
> 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().
Well spotted! It's definitely a bug. Would be very surprising if the
implementation tried https for an URI starting with "WS://" :-)
> 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.
Done.
> 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?
(see comments above)
> src/java.httpclient/share/classes/java/net/http/SequentialExecutor.java
>
> Maybe the lines 9-12 should be converted to a JavaDocs comment.
I may be mistaken, but I feel a proper javadoc comment would be justified in
case of a highly reusable or public component. Neither of those are applicable
to any of the implementation types here.
Who would read these javadocs?
> 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.
(see the comments above)
> 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.
I've tried to adhere to this:
http://cr.openjdk.java.net/~alundblad/styleguide/#toc-indentation
Might revisit later. I will wait for the status change of this document.
> 120 throw new InternalError(String.valueOf(s));
>
> Could you please provide a better message for exception here?
(see comments above)
> 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.:
I thought about it. But then we might have an ambiguity. One might create
different versions of the object with the same numerical code, but different
descriptions. Should they be equal? If they are gonna be cached, then which one?
Overall, it might kill this class' property of being value-based.
If a non-standard Close Code would be created, it would be created by the
client, rather than by the server. So a client already knows what the code
means.
> public static CloseCode of(int code, String description)
>
> 1221 return Objects.hash(code);
>
> I think it should be changed to return just the code.
Done (just don't expect it to be specified in the javadoc).
> 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.
Done.
> 74 if (forbiddenHeadersLowerCased.contains(name.toLowerCase())) {
>
> Please use name.toLowerCase(Locale.ROOT).
Done.
> 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.
Done.
> src/java.httpclient/share/classes/java/net/http/WebSocketHandshakeException.java
>
> 36 private transient HttpResponse response;
>
> The field can be final.
Done. Thanks!
> src/java.httpclient/share/classes/java/net/http/WebSocketImpl.java
>
> 310 throw new IllegalStateException();
(see comments above)
> 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.
Done.
> 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.
Done.
> test/java/net/httpclient/HandshakePhase.java
>
> The copyright header is missed.
Good catch! Thanks.
All the changes can be seen at the old url (I've done them in place):
http://cr.openjdk.java.net/~prappo/8087113/webrev.03/
--------------------------------------------------------------------------------
[1] https://bugs.openjdk.java.net/browse/JDK-8085796
More information about the net-dev
mailing list