Issue #4772 - support partial messages for Jetty WS API annotations #6357
Conversation
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
...socket/websocket-jetty-api/src/main/java/org/eclipse/jetty/websocket/api/RemoteEndpoint.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
...-jetty-api/src/main/java/org/eclipse/jetty/websocket/api/annotations/OnWebSocketMessage.java
Outdated
Show resolved
Hide resolved
| * for the {@link java.nio.ByteBuffer} type as no copying needs to occur to get the data into this format.</p> | ||
| * <ol> | ||
| * <li><code>public void methodName(ByteBuffer payload, boolean last)</code></li> | ||
| * <li><code>public void methodName(byte payload, boolean last)</code></li> |
There was a problem hiding this comment.
Isn't this byte[] in the signature?
Frankly, I would not offer a byte[] variant.
Also, what do you do with the ByteBuffer version if it is retained by the application, but not consumed? I think the signature needs a Callback to signal when the ByteBuffer can be released, no?
If you choose for no Callback, then you have to remove from the javadocs above "as no copying needs to occur to get the data", and specify precisely that applications must consume the buffer before returning from the method.
There was a problem hiding this comment.
Also, what do you do with the ByteBuffer version if it is retained by the application, but not consumed? I think the signature needs a Callback to signal when the ByteBuffer can be released, no?
The websocket-core API has this feature but for the Jetty and Javax APIs they don't take callbacks, so you only own the ByteBuffer only until the method returns. If you wish to retain the ByteBuffer you must copy it.
If you choose for no Callback, then you have to remove from the javadocs above "as no copying needs to occur to get the data"
By this I meant that ByteBuffer can be passed into the method directly and doesn't need to be copied by the implementation, but this is not the case for byte[] or String and there will be some copying involved to get it into that format. I didn't mean that you can retain the ByteBuffer indefinitely without copying it.
and specify precisely that applications must consume the buffer before returning from the method.
They don't have to consume the buffer, if they want to ignore the rest of the message they don't need to consume it. And this is no different to the whole message signatures.
I have removed the byte[] option and this part of the Javadoc so it is less confusing.
There was a problem hiding this comment.
By this I meant that ByteBuffer can be passed into the method directly and doesn't need to be copied by the implementation, but this is not the case for byte[] or String and there will be some copying involved to get it into that format. I didn't mean that you can retain the ByteBuffer indefinitely without copying it.
This is the javadoc for users -- the implementation details should be left out.
As a user, though, I must know if I have to copy the buffer (assuming interest in the content) before returning from the method.
...-jetty-api/src/main/java/org/eclipse/jetty/websocket/api/annotations/OnWebSocketMessage.java
Outdated
Show resolved
Hide resolved
...mmon/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerFactory.java
Show resolved
Hide resolved
...etty-tests/src/test/java/org/eclipse/jetty/websocket/tests/AnnotatedPartialListenerTest.java
Show resolved
Hide resolved
…ages. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
| * </ol> | ||
| * <p>Note: Similar to the signatures above these can all be used with an optional first {@link Session} parameter.</p> |
There was a problem hiding this comment.
I think you should add a note about split UTF-8 characters... I'm guessing the payload always contains full UTF-8 sequences and you keep split UTF-8 bytes around for the next call?
There was a problem hiding this comment.
Also, what happens if the message becomes bad during a partial message delivery.
Eg: first 2 calls to methodName(String, boolean) are fine, but the 3rd call cannot be made as the ws-core found the message to be in violation of the websocket messaging rules.
| InvalidDoubleTextListener doubleTextListener = new InvalidDoubleTextListener(); | ||
| assertThrows(InvalidWebSocketException.class, () -> client.connect(doubleTextListener, serverUri)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you please add one more test for split UTF-8 bytes?
There was a problem hiding this comment.
and late bad message / utf-8 as well.
- good partial sequence
- good partial sequence
- bad partial utf-8 sequence
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Closes #4772.
Adds support for partial messages to the Jetty WebSocket API
@OnWebSocketMessageannotation.