Skip to content
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

Issue #4824 - add configuration on RemoteEndpoint for maxOutgoingFrames #5059

Merged
merged 7 commits into from Sep 10, 2020

Conversation

lachlan-roberts
Copy link
Contributor

Attempt to address issue #4824 at the Jetty WebSocket API level.

If there are more than maxOutgoingFrames waiting to send, any additional sends will fail immediately instead of continuing to queue up in the flusher. The callback will be failed immediately but it will not fail the entire ws connection.

This method is called maxOutgoingFrames but its not really as the implementation could later on decide to fragment each send from the WebSocketRemoteEndpoint into multiple frames.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

Maybe it would be better if instead we monitor how many frames are queued at a lower level and if this is exceeded we simply just fail the entire connection.

@joakime joakime added this to In progress in Jetty 9.4.32 via automation Jul 30, 2020
@joakime joakime linked an issue Jul 30, 2020 that may be closed by this pull request
@joakime
Copy link
Contributor

joakime commented Jul 30, 2020

I can see the value in this, and I'm good with this implementation.
But I'm wondering if there is a better way to accomplish this.

@gregw @sbordet thoughts?

@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.32 Aug 31, 2020
@joakime
Copy link
Contributor

joakime commented Sep 1, 2020

@lachlan-roberts i think this PR needs a refresh / merge from jetty-9.4.x

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that shows that maxOutgoingFrames is respected.

callback = from(callback, numOutgoingFrames::decrementAndGet);
if (outgoingFrames > maxNumOutgoingFrames)
{
callback.writeFailed(new IOException("Exceeded max outgoing frames: " + outgoingFrames + ">" + maxNumOutgoingFrames));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the exception from IOException to WritePendingException.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Jetty 9.4.32 automation moved this from Review in progress to Reviewer approved Sep 4, 2020
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - just improve the javadocs.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts merged commit 1256261 into jetty-9.4.x Sep 10, 2020
Jetty 9.4.32 automation moved this from Reviewer approved to Done Sep 10, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-9.4.x-4824-WSmaxOutgoingFrames branch September 10, 2020 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.32
  
Done
Development

Successfully merging this pull request may close these issues.

WebSocket server outgoing message queue memory growth
3 participants