search

jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.86k stars 1.91k forks source link

Improve WebSocket connection closing #12235

Open sbordet opened 1 month ago

sbordet commented 1 month ago

Jetty version(s) 12

Description The analysis to fix #11581 showed the following:

WebSocket closes the EndPoint too early, namely when it has received a CLOSE frame and it has sent one.

Instead, it should wait to read -1 after receiving the CLOSE frame, and close the EndPoint after it has sent one.

When WebSocket is carried over HTTP/2, the CLOSE frame is carried in a DATA frame with endStream=false, followed by an empty DATA frame with endStream=true.

If the HTTP2StreamEndPoint is closed too early, it results in the empty DATA frame with endStream=true to be seen as an invalid frame arriving after the HTTP2Stream has been closed, resulting in a RST_STREAM being sent to the other peer.

The other peer then receives: a DATA frame containing the WebSocket CLOSE frame, then the empty DATA frame, then a RST_STREAM frame. The DATA frame containing the WebSocket CLOSE frame typically triggers the WebSocket implementation to read from the EndPoint in a different thread, but the read may be delayed. If the read is delayed, the RST_STREAM frame may be processed, which drains the DATA frames, so that the WebSocket implementation only reads an unexpected -1, rather than the WebSocket CLOSE frame.

This is a general issue for protocols carried over HTTP/2: they must read up to -1 before triggering the close of the EndPoint, despite the fact that a previous read signaled the protocol "end-of-data".

sbordet commented 1 month ago

Furthermore, JettyWebSocketFrameHandler.notifyClose() can be invoked multiple times, but it should really be invoked once.

The protection about calling it once should be both on the read side (when receiving a CLOSE frame) and on the write side (when closing the WebSocket Session) which likely requires grabbing a coarse lock (but only for the close handling).

Currently, JettyWebSocketFrameHandler.closeNotified protects from multiple calls, but fails the callback, which results in a failure path for what should instead be a noop.