-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
websocket protocol support #73
base: master
Are you sure you want to change the base?
Conversation
Copyright header is missing in all the files. |
You might want to add yourself to |
|
||
public void handshake(Request request) throws IOException { | ||
final String version = request.getHeader(WebSocketHeaders.VERSION); | ||
if (!"13".equals(version)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic constant?
server.handleMessage(session, (BinaryMessage) message); | ||
} else if (message instanceof CloseMessage) { | ||
server.handleMessage(session, (CloseMessage) message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe IllegalArgumentException in case of unexpected message?
@Override | ||
public void close() { | ||
try { | ||
extensions.forEach(Extension::close); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to get some exception from Extension.close and stay with some unclosed ones?
return response; | ||
} catch (VersionException e) { | ||
Response response = new Response("426 Upgrade Required", Response.EMPTY); | ||
response.addHeader(WebSocketHeaders.createVersionHeader(13)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic constant once again.
/** | ||
* @author <a href="mailto:[email protected]">Vadim Yelisseyev</a> | ||
*/ | ||
public class HandshakeException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to extend WebSocketException
here as siblings do?
} | ||
|
||
public Map<String, String> getParameters() { | ||
return parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wrap to unmodifiableMap?
*/ | ||
public class ExtensionRequestParser { | ||
|
||
public static List<ExtensionRequest> parse(String header) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unit test for this parsing would be valuable.
if (serverContextTakeover) { | ||
serverContextTakeover = false; | ||
} else { | ||
throw new HandshakeException("Duplicate definition of the server_no_context_takeover extension parameter"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a HandshakeException?
|
||
@Override | ||
public String toString() { | ||
return getClass().getSimpleName() + "<" + Hex.toHex(payload) + ">"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long this payload can be? Is it useful to create such a string for a long payload?
public static final short TLS_HANDSHAKE_FAILURE = 1015; | ||
|
||
public CloseMessage(byte[] payload) { | ||
this(payload.length == 0 ? null : Short.reverseBytes(unsafe.getShort(payload, byteArrayOffset))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite hard to perceive unsafe
due to static import.
JavaInternals.unsafe.getShort
seems to be more understandable.
|
||
@Override | ||
public String toString() { | ||
return "CloseMessage<" + payload + ">"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raw byte[].toString?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, it's short here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a number of questions and comments. PR contains complex logic, so unit tests for different types of messages, serde, fragmentation, size limits, options, etc. are definitely welcome.
response.addHeader("Connection: Upgrade"); | ||
response.addHeader(WebSocketHeaders.createAcceptHeader(request)); | ||
return response; | ||
} catch (VersionException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where can it come from?
} | ||
|
||
protected void handleMessage(WebSocketSession session, Message<?> message) throws IOException { | ||
if (message instanceof PingMessage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use message.opcode
to replace numerous instanceof
s with faster switch
on enum
?
|
||
@Override | ||
public void handleException(Throwable e) { | ||
log.error(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving logging inside if
? super.handleException()
deals with logging already.
|
||
@Override | ||
public String toString() { | ||
return getClass().getSimpleName() + "<" + Hex.toHex(payload) + ">"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SimpleName.of()
is generally faster and more predictable.
private final boolean fin; | ||
private final Opcode opcode; | ||
private int rsv; | ||
private int payloadLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this field if we have payload.length()
?
private static final String SERVER_NO_CONTEXT_TAKEOVER = "server_no_context_takeover"; | ||
private static final String CLIENT_NO_CONTEXT_TAKEOVER = "client_no_context_takeover"; | ||
|
||
private static final int RSV_BITMASK = 0b100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be helpful.
private static final String CLIENT_NO_CONTEXT_TAKEOVER = "client_no_context_takeover"; | ||
|
||
private static final int RSV_BITMASK = 0b100; | ||
private static final byte[] EOM_BYTES = new byte[] {0, 0, -1, -1}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be helpful.
break; | ||
} | ||
} else { | ||
if (fin && !clientContextTakeover) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests especially for context takeover and framing would be appreciated.
|
||
int compressedLength; | ||
do { | ||
compressedLength = deflater.deflate(outputBuffer, 0, outputBuffer.length, Deflater.SYNC_FLUSH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JavaDoc says:
In the case of FULL_FLUSH or SYNC_FLUSH, if the return value is len, the space available in output buffer b, this method should be invoked again with the same flush parameter and more output space.
My concern is about a case when compressed payload
doesn't fit outputBuffer
.
if (protocols != null) { | ||
for (String protocol : protocols.split(",")) { | ||
if (config.isSupportedProtocol(protocol)) { | ||
response.addHeader(WebSocketHeaders.PROTOCOL + protocol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROTOCOL header value from request
is transformed into multiple headers in response
-- is that the desired behavior?
WebSocket protocol support