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

WebSocket - Handling sent 1009 close frame. #3666

Closed
IlyaZu opened this issue May 16, 2019 · 7 comments
Closed

WebSocket - Handling sent 1009 close frame. #3666

IlyaZu opened this issue May 16, 2019 · 7 comments

Comments

@IlyaZu
Copy link

IlyaZu commented May 16, 2019

If Jetty "server" sends a large message (1GB in my tests) via WebSocket that causes the recipient to return a 1009 close frame - Jetty does not send the response close frame back. (As far as I can tell, it gets queued for sending behind the large message.) As a result the server's connection stays open until it times out naturally.

While this is technically to WebSocket speck - it's not ideal.

Am I missing something? Can this be addressed in a later version?

Thank you for your time :)

Using Jetty 9.4.18.v20190429.

@joakime
Copy link
Contributor

joakime commented May 16, 2019

The close frame sits after the current active message.
In other words once the data frame with fin==true is sent, then the close frame is sent.
This is to spec, as you pointed out.
But the alternative, having Jetty immediately close the connection has caused more issues.
For this situation we encourage applications to have a valid idle timeout configured.

But lets talk about other possible future options ...

Close location in outgoing frame queue depends on remote received close frame type?
But that wouldn't work if the current frame hasn't been fully flushed yet, and the remote isn't reading.
That close frame would just sit there, waiting for the current frame to complete.

Perhaps it would make more sense to allow the application to decide how to handle close?
One possible way would be to have the application implement an OnClose handler method.
In that method the application can choose to respond with a close of its own, or just disconnect the connection (WebSocketSession.disconnect()) without a close handshake.
If the application's onClose submits a close of its own, then that goes to the front of the queue, otherwise the implementation adds it to the end of the queue.
But even this doesn't help for the same reasons as above, if the current frame hasn't been fully flushed yet, and the remote isn't reading, then that close frame would just sit there.

@joakime
Copy link
Contributor

joakime commented May 16, 2019

I wonder what would happen if your application had an OnClose handler method that reset the idle timeout to something short?

@IlyaZu
Copy link
Author

IlyaZu commented May 16, 2019

Thanks for your response, joakime.

Hmm. Yeah, it might be possible to set timeout to something short on WebSocketListener.onWebSocketClose() for my application - I'll have a look at doing that tomorrow.

It's still a bit wonky because the other side will never receive a close frame in response - but I guess it has to deal with similar situations anyway.

Cheers

@IlyaZu
Copy link
Author

IlyaZu commented May 17, 2019

Setting session.setIdleTimeout() in WebSocketListener.onWebSocketClose() seems like a decent solution for the side receiving the 1009 frame.

Is there a good way to catch when a Jetty is sending a close frame? My thinking is that the sender should also set its timeout to something short since it can't guarantee that it will get the close frame back in a timely fashion.

As a (hopefully) interesting aside - it seems like Tomcat handles replying to a 1009 frame more gracefully (the close frame is responded to and large data frame write generates an exception). Don't know how comparable Tomcat is to Jetty but might be worth comparing notes?

@gregw
Copy link
Contributor

gregw commented May 17, 2019

I don't understand how the close frame can be responded to? Once the 1GB message starts to be sent, then there is nothing that the sender can do to abort that prior to sending all the 1GB of payload... unless the 1GB is fragmented into many smaller frames, then the close can be sent after any of those frames.... but the other end has to keep consuming those frames else we will get blocked.

We already have a mechanism for some frames (eg ping) to jump the queue of messages. Perhaps we need to handle 1009 in the same way - if we receive one, then we should queue a close reply at the start of the write queue and fail any frames in the queue behind it.

We have to be careful about what close codes would have this behaviour as we don't want to break graceful close that would happen after all queued frames are sent.

@lachlan-roberts your thoughts about jetty-10?

@lachlan-roberts
Copy link
Contributor

@gregw Maybe if we receive a close frame which is an error status code, we could wrap this as an abnormal close frame. Then in the FrameFlusher we get the abnormal close frame, empty the queue and fail all the frames, then add the close frame to the queue.

You could use the FragmentExtension so you are not blocked behind a large frame to send but we might still have multiple frames waiting to be written in the entries list, so I am not sure how effective this will be.

@gregw
Copy link
Contributor

gregw commented May 21, 2019

@lachlan-roberts I think a 1009 is an abnormal close, so if we already have logic to handle that, then yes - lets trigger it! Make it so!

lachlan-roberts added a commit to lachlan-roberts/jetty.project that referenced this issue May 22, 2019
…queue

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit to lachlan-roberts/jetty.project that referenced this issue May 24, 2019
- code cleanups
- made SHUTDOWN 1001 status an abnormal close status

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit to lachlan-roberts/jetty.project that referenced this issue May 24, 2019
…queue

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit to lachlan-roberts/jetty.project that referenced this issue May 24, 2019
- code cleanups
- made SHUTDOWN 1001 status an abnormal close status

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit to lachlan-roberts/jetty.project that referenced this issue Jun 3, 2019
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jun 4, 2019
…Close

Issue #3666 - error CloseFrames skip frames in the FrameFlusher queue
@IlyaZu IlyaZu closed this as completed Jun 18, 2019
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

No branches or pull requests

4 participants