Issue #9944 - remove integer from demand in websocket core #9945
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
...ebsocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/WebSocketConnection.java
Show resolved
Hide resolved
...socket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java
Show resolved
Hide resolved
| @@ -152,14 +152,13 @@ public interface CoreSession extends OutgoingFrames, IncomingFrames, Configurati | |||
| void abort(); | |||
|
|
|||
| /** | |||
| * <p>Manages flow control by indicating demand for WebSocket frames.</p> | |||
| * <p>Manages flow control by indicating demand for a WebSocket frame.</p> | |||
There was a problem hiding this comment.
The behaviour for calling demand when there is already pending demand needs to be documented. You are currently internally tracking a long count, so you have effectively kept the long and could reproduce with:
public void demand(long n)
{
for (long i = 0; i < n; i++)
demand();
}I'm not sure this is optimal. Could we throw ISE if there is already pending demand?
There was a problem hiding this comment.
I still think you need to javadoc in this PR what is the expected behaviour is of a demand call with pending demand... even if the actual behaviour is implemented in a different PR.
There was a problem hiding this comment.
Should it be ReadPendingException or IllegalStateException?
There was a problem hiding this comment.
ReadPendingException extends IllegalStateException so either works.
| @@ -152,14 +152,13 @@ public interface CoreSession extends OutgoingFrames, IncomingFrames, Configurati | |||
| void abort(); | |||
|
|
|||
| /** | |||
| * <p>Manages flow control by indicating demand for WebSocket frames.</p> | |||
| * <p>Manages flow control by indicating demand for a WebSocket frame.</p> | |||
There was a problem hiding this comment.
I still think you need to javadoc in this PR what is the expected behaviour is of a demand call with pending demand... even if the actual behaviour is implemented in a different PR.
Signed-off-by: Lachlan Roberts <[email protected]>
| @@ -152,14 +152,13 @@ public interface CoreSession extends OutgoingFrames, IncomingFrames, Configurati | |||
| void abort(); | |||
|
|
|||
| /** | |||
| * <p>Manages flow control by indicating demand for WebSocket frames.</p> | |||
| * <p>Manages flow control by indicating demand for a WebSocket frame.</p> | |||
There was a problem hiding this comment.
ReadPendingException extends IllegalStateException so either works.
| @@ -110,7 +110,8 @@ protected void demand() | |||
| Frame frame = serverHandler.receivedFrames.poll(250, TimeUnit.MILLISECONDS); | |||
| assertNull(frame); | |||
|
|
|||
| serverHandler.getCoreSession().demand(2); | |||
| serverHandler.getCoreSession().demand(); | |||
| serverHandler.getCoreSession().demand(); | |||
There was a problem hiding this comment.
This seems wrong, as the second call should throw.
If it's not causing flakyness, needs a comment explaining why.
There was a problem hiding this comment.
See comments from #9945 (comment)
I have opened #9965 to change the implementation.
There was a problem hiding this comment.
In the next PR these should fail and I will need to change them.
Hopefully that will make beta3 as well, but if not at least we have the API changed with this.
| coreSession.demand(3); | ||
| coreSession.demand(); | ||
| coreSession.demand(); | ||
| coreSession.demand(); |
| @@ -307,7 +310,8 @@ public void onOpen(CoreSession coreSession, Callback callback) | |||
| { | |||
| super.onOpen(coreSession); | |||
| callback.succeeded(); | |||
| coreSession.demand(2); | |||
| coreSession.demand(); | |||
| coreSession.demand(); | |||
| @@ -375,7 +379,8 @@ public void onOpen(CoreSession coreSession, Callback callback) | |||
| { | |||
| super.onOpen(coreSession); | |||
| callback.succeeded(); | |||
| coreSession.demand(2); | |||
| coreSession.demand(); | |||
| coreSession.demand(); | |||
Remove the
longargument fromdemand()in websocket core.This had already been done for the new core Jetty WebSocket API.