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 #3666 - error CloseFrames skip frames in the FrameFlusher queue #3686

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented May 22, 2019

#3666
if the FrameFlusher receives a CloseFrame with an error status it will not queue these frames behind the frames already queued in the FrameFlusher, but will fail all frames waiting to be sent and send the error CloseFrame instead

@sbordet
Copy link
Contributor

sbordet commented May 22, 2019

@lachlan-roberts I don't think this is right.

It is perfectly acceptable for an application to send a message and then close, with the intent to deliver both the message and the close.
From what I understand of this PR you queue the message, but when you queue the close it will fail the message and this is not right.

What is the reason behind this change?

@gregw
Copy link
Contributor

gregw commented May 22, 2019

@sbordet read #3666
@lachlan-roberts I agree that we should abort queued frames when we are responding to an abnormal close, but should we do so if we are initiating an abnormal close??? hmmm probably yes.... if we are sending a long queue of frames and we suddenly receive a message that is too large to handle and we want to close with a 1009, then I guess we should not wait until we have flushed our queue of messages before abnormally closing.

…queue

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
- code cleanups
- made SHUTDOWN 1001 status an abnormal close status

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

sorry had to rebase to fix failures from the WebSocketChannel rename

@lachlan-roberts
Copy link
Contributor Author

@gregw @sbordet can I get a re-review?

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

almost....

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts merged commit f282ba4 into jetty:jetty-10.0.x Jun 4, 2019
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-3666-errorClose branch July 1, 2019 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants