websocket protocol support #73

twister55 · 2023-02-09T20:17:19Z

WebSocket protocol support

incubos · 2023-02-13T09:30:20Z

Copyright header is missing in all the files.

incubos · 2023-02-13T09:30:56Z

You might want to add yourself to CONTRIBUTORS.

dormidon · 2023-02-13T12:29:33Z

src/one/nio/ws/WebSocketSession.java

+
+    public void handshake(Request request) throws IOException {
+        final String version = request.getHeader(WebSocketHeaders.VERSION);
+        if (!"13".equals(version)) {


Magic constant?

dormidon · 2023-02-13T12:31:12Z

src/one/nio/ws/WebSocketSession.java

+            server.handleMessage(session, (BinaryMessage) message);
+        } else if (message instanceof CloseMessage) {
+            server.handleMessage(session, (CloseMessage) message);
+        }


Maybe IllegalArgumentException in case of unexpected message?

dormidon · 2023-02-13T12:33:02Z

src/one/nio/ws/WebSocketSession.java

+    @Override
+    public void close() {
+        try {
+            extensions.forEach(Extension::close);


Is it possible to get some exception from Extension.close and stay with some unclosed ones?

fixed, thanks

dormidon · 2023-02-13T12:35:28Z

src/one/nio/ws/WebSocketSession.java

+            return response;
+        } catch (VersionException e) {
+            Response response = new Response("426 Upgrade Required", Response.EMPTY);
+            response.addHeader(WebSocketHeaders.createVersionHeader(13));


Magic constant once again.

dormidon · 2023-02-13T12:39:52Z

src/one/nio/ws/exception/HandshakeException.java

+/**
+ * @author <a href="mailto:vadim.yelisseyev@gmail.com">Vadim Yelisseyev</a>
+ */
+public class HandshakeException extends RuntimeException {


Is it possible to extend WebSocketException here as siblings do?

I think the handshake happens before the request goes from HTTP to WebSocket state so I separated them in two different hierarchies. But I don't mind to join it :)

dormidon · 2023-02-13T12:40:44Z

src/one/nio/ws/extension/ExtensionRequest.java

+    }
+
+    public Map<String, String> getParameters() {
+        return parameters;


Maybe wrap to unmodifiableMap?

dormidon · 2023-02-13T12:43:06Z

src/one/nio/ws/extension/ExtensionRequestParser.java

+ */
+public class ExtensionRequestParser {
+
+    public static List<ExtensionRequest> parse(String header) {


It seems unit test for this parsing would be valuable.

dormidon · 2023-02-13T12:45:45Z

src/one/nio/ws/extension/PerMessageDeflate.java

+                if (serverContextTakeover) {
+                    serverContextTakeover = false;
+                } else {
+                    throw new HandshakeException("Duplicate definition of the server_no_context_takeover extension parameter");


Is it really a HandshakeException?

I looked all usages of HandshakeException and for now it's always threw for illegal arguments in request. I can remove it in favor of IllegalArgumentException or I can make to HandshakeException extends IllegalArgumentException. What do you think?

сс @incubos

dormidon · 2023-02-13T13:00:22Z

src/one/nio/ws/message/BinaryMessage.java

+
+    @Override
+    public String toString() {
+        return getClass().getSimpleName() + "<" + Hex.toHex(payload) + ">";


How long this payload can be? Is it useful to create such a string for a long payload?

dormidon · 2023-02-13T13:05:38Z

src/one/nio/ws/message/CloseMessage.java

+    public static final short TLS_HANDSHAKE_FAILURE = 1015;
+
+    public CloseMessage(byte[] payload) {
+        this(payload.length == 0 ? null : Short.reverseBytes(unsafe.getShort(payload, byteArrayOffset)));


It's quite hard to perceive unsafe due to static import.
JavaInternals.unsafe.getShort seems to be more understandable.

dormidon · 2023-02-13T13:06:08Z

src/one/nio/ws/message/CloseMessage.java

+
+    @Override
+    public String toString() {
+        return "CloseMessage<" + payload + ">";


Raw byte[].toString?

Oh, sorry, it's short here.

incubos

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.

incubos · 2023-02-13T10:10:32Z

src/one/nio/ws/WebSocketSession.java

+            response.addHeader("Connection: Upgrade");
+            response.addHeader(WebSocketHeaders.createAcceptHeader(request));
+            return response;
+        } catch (VersionException e) {


Where can it come from?

incubos · 2023-02-13T10:13:43Z

src/one/nio/ws/WebSocketSession.java

+    }
+
+    protected void handleMessage(WebSocketSession session, Message<?> message) throws IOException {
+        if (message instanceof PingMessage) {


Is it possible to use message.opcode to replace numerous instanceofs with faster switch on enum?

incubos · 2023-02-13T10:15:22Z

src/one/nio/ws/WebSocketSession.java

+
+    @Override
+    public void handleException(Throwable e) {
+        log.error(e);


What do you think about moving logging inside if? super.handleException() deals with logging already.

sure, thanks

incubos · 2023-02-13T11:59:37Z

src/one/nio/ws/message/BinaryMessage.java

+
+    @Override
+    public String toString() {
+        return getClass().getSimpleName() + "<" + Hex.toHex(payload) + ">";


SimpleName.of() is generally faster and more predictable.

fixed, thanks

incubos · 2023-02-13T12:52:22Z

src/one/nio/ws/frame/Frame.java

+    private final boolean fin;
+    private final Opcode opcode;
+    private int rsv;
+    private int payloadLength;


Do we really need this field if we have payload.length()?

first payloadLength is read from the frame header and then the number of bytes equal to payloadLength is read from the socket, so we need to store this field separately (see FrameReader.java:65)

incubos · 2023-02-14T05:22:07Z

src/one/nio/ws/extension/PerMessageDeflate.java

+    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;


A comment would be helpful.

incubos · 2023-02-14T05:22:11Z

src/one/nio/ws/extension/PerMessageDeflate.java

+    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};


A comment would be helpful.

incubos · 2023-02-14T05:36:32Z

src/one/nio/ws/extension/PerMessageDeflate.java

+                            break;
+                        }
+                    } else {
+                        if (fin && !clientContextTakeover) {


Unit tests especially for context takeover and framing would be appreciated.

incubos · 2023-02-14T05:41:40Z

src/one/nio/ws/extension/PerMessageDeflate.java

+
+            int compressedLength;
+            do {
+                compressedLength = deflater.deflate(outputBuffer, 0, outputBuffer.length, Deflater.SYNC_FLUSH);


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.

incubos · 2023-02-14T06:10:08Z

src/one/nio/ws/WebSocketSession.java

+        if (protocols != null) {
+            for (String protocol : protocols.split(",")) {
+                if (config.isSupportedProtocol(protocol)) {
+                    response.addHeader(WebSocketHeaders.PROTOCOL + protocol);


PROTOCOL header value from request is transformed into multiple headers in response -- is that the desired behavior?

there is a break on next line, so only first supported protocol will be taken

twister55 added 2 commits February 9, 2023 23:02

websocket protocol support

c301eef

echo server client

104d331

incubos requested review from incubos and dormidon February 13, 2023 09:19

dormidon reviewed Feb 13, 2023

View reviewed changes

incubos requested changes Feb 14, 2023

View reviewed changes

incubos self-assigned this Mar 24, 2023

twister55 and others added 11 commits September 1, 2024 13:57

Merge branch 'odnoklassniki:master' into websockets

d64d9f1

added copyright header to all new files

23d095a

review fixes

3243de2

removed TextMessage constructor from bytes

df073ae

added comment to message reader

7237490

added logging

f6df10b

added comment about EOM_BYTES

0f0b3b8

close message payload fix

d9be599

review fixes

f82220e

removed unsafe usage

da84274

fix for closing extensions

a7c349a

Vadim Yelisseyev added 4 commits September 14, 2024 17:31

buffer size moved to config (Integer.getInteger)

0b6058b

review fix

f8d9a4d

fix

e8300a4

added comment

af9921f

websocket protocol support #73

Are you sure you want to change the base?

websocket protocol support #73

Conversation

twister55 commented Feb 9, 2023

WebSocket protocol support

incubos commented Feb 13, 2023

incubos commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twister55 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incubos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twister55 Sep 14, 2024 •

edited

Loading