From f54d714afd57d70de2c3978f2ca51a4e78a1c25f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Laignel?= Date: Sun, 21 Jan 2024 20:34:29 +0100 Subject: [PATCH] webrtc: only use close() to close websockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the signaller clients and servers, the following sequence is used to close the websocket (in the [send task]): ```rust ws_sink.send(WsMessage::Close(None)).await?; ws_sink.close().await?; ``` tungstenite's [`WebSocket::close()` doc] states: > Calling this function is the same as calling `write(Message::Close(..))`` So we might think they are redundant and either could be used for this purpose (`send()` calls `write()`, then `flush()`). The result is actually is bit different as `write()` starts by checking the state of the connection and [returns `SendAfterClosing`] if the socket is no longer active, which is the case when a closing request has been received from the peer via a [call to `do_close()`]). Note that `do_close()` also enqueues a `Close` frame. This behaviour is visible from the server's logs: ``` 1. tungstenite::protocol: Received close frame: None 2. tungstenite::protocol: Replying to close with Frame { header: FrameHeader { .., opcode: Control(Close), .. }, payload: [] } 3. gst_plugin_webrtc_signalling::server: Received message Ok(Close(None)) 4. gst_plugin_webrtc_signalling::server: connection closed: None this_id=cb13892f-b4d5-4d59-95e2-b3873a7bd319 5. remove_peer{peer_id="cb13892f-b4d5-4d59-95e2-b3873a7bd319"}: gst_plugin_webrtc_signalling::server: close time.busy=285µs time.idle=55.5µs 6. async_tungstenite: websocket start_send error: WebSocket protocol error: Sending after closing is not allowed ``` 1: The server's websocket receives the peer's `Close(None)`. 2: `do_close()` enqueues a `Close` frame. 3: The incoming `Close(None)` is handled by the server. 4 & 5: perform session closing. 6: `ws_sink.send(WsMessage::Close(None))` attempts to `write()` while the ws is no longer active. The error causes an early return, which means that the enqueued `Close` frame is not flushed. Depending on the peer's shutdown sequence, this can result in the following error, which can bubble up as a `Message` on the application's bus: ``` ERROR: from element /GstPipeline:pipeline0/GstWebRTCSrc:webrtcsrc0: GStreamer encountered a general stream error. Additional debug info: net/webrtc/src/webrtcsrc/imp.rs(625): gstrswebrtc::webrtcsrc::imp::BaseWebRTCSrc::connect_signaller::{{closure}}::{{closure}} (): /GstPipeline:pipeline0/GstWebRTCSrc:webrtcsrc0: Signalling error: Error receiving: WebSocket protocol error: Connection reset without closing handshake ``` On the other hand, [`close()` ensures the ws is active] before attempting to write a `Close` frame. If it's not, it only flushes the stream. Thus, when we want to be able to close the websocket and/or to honor the closing handshake in response to the peer `Close` message, the `ws_sink.close()` variant is preferable. This can be verified in the resulting server's logs: ``` tungstenite::protocol: Received close frame: None tungstenite::protocol: Replying to close with Frame { header: FrameHeader { is_final: true, rsv1: false, rsv2: false, rsv3: false, opcode: Control(Close), mask: None}, payload: [] } gst_plugin_webrtc_signalling::server: Received message Ok(Close(None)) gst_plugin_webrtc_signalling::server: connection closed: None this_id=192ed7ff-3b9d-45c5-be66-872cbe67d190 remove_peer{peer_id="192ed7ff-3b9d-45c5-be66-872cbe67d190"}: gst_plugin_webrtc_signalling::server: close time.busy=22.7µs time.idle=37.4µs tungstenite::protocol: Sending pong/close ``` We now get the notification `Sending pong/close` (the closing handshake) instead of `websocket start_send error` from step 6 with previous variant. The `Connection reset without closing handshake` was not observed after this change. [send task]: https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/blob/63b568f4a0d4d38377d4dd16f9d1d508abad0d85/net/webrtc/signalling/src/server/mod.rs#L165 [`WebSocket::close()` doc]: https://docs.rs/tungstenite/0.21.0/tungstenite/protocol/struct.WebSocket.html#method.close [returns `SendAfterClosing`]: https://github.com/snapview/tungstenite-rs/blob/85463b264e3f672ef2004294d82fd3f4ee6a8ca3/src/protocol/mod.rs#L437 [call to `do_close()`]: https://github.com/snapview/tungstenite-rs/blob/85463b264e3f672ef2004294d82fd3f4ee6a8ca3/src/protocol/mod.rs#L601 [`close()` ensures the ws is active]: https://github.com/snapview/tungstenite-rs/blob/85463b264e3f672ef2004294d82fd3f4ee6a8ca3/src/protocol/mod.rs#L531 Part-of: --- net/webrtc/signalling/src/server/mod.rs | 1 - net/webrtc/src/aws_kvs_signaller/imp.rs | 1 - net/webrtc/src/janusvr_signaller/imp.rs | 1 - net/webrtc/src/signaller/imp.rs | 1 - 4 files changed, 4 deletions(-) diff --git a/net/webrtc/signalling/src/server/mod.rs b/net/webrtc/signalling/src/server/mod.rs index e0b034a3..700ae4dd 100644 --- a/net/webrtc/signalling/src/server/mod.rs +++ b/net/webrtc/signalling/src/server/mod.rs @@ -162,7 +162,6 @@ impl Server { } } - ws_sink.send(WsMessage::Close(None)).await?; ws_sink.close().await?; Ok::<(), Error>(()) diff --git a/net/webrtc/src/aws_kvs_signaller/imp.rs b/net/webrtc/src/aws_kvs_signaller/imp.rs index 254434a0..7946e244 100644 --- a/net/webrtc/src/aws_kvs_signaller/imp.rs +++ b/net/webrtc/src/aws_kvs_signaller/imp.rs @@ -457,7 +457,6 @@ impl Signaller { gst::info!(CAT, imp: imp, "Done sending"); } - ws_sink.send(WsMessage::Close(None)).await?; ws_sink.close().await?; Ok::<(), Error>(()) diff --git a/net/webrtc/src/janusvr_signaller/imp.rs b/net/webrtc/src/janusvr_signaller/imp.rs index 72039420..56d6a458 100644 --- a/net/webrtc/src/janusvr_signaller/imp.rs +++ b/net/webrtc/src/janusvr_signaller/imp.rs @@ -314,7 +314,6 @@ impl Signaller { |this| gst::info!(CAT, imp: this, "{msg}") ); - ws_sink.send(WsMessage::Close(None)).await?; ws_sink.close().await?; Ok::<(), Error>(()) diff --git a/net/webrtc/src/signaller/imp.rs b/net/webrtc/src/signaller/imp.rs index 18c1a148..fb73c053 100644 --- a/net/webrtc/src/signaller/imp.rs +++ b/net/webrtc/src/signaller/imp.rs @@ -152,7 +152,6 @@ impl Signaller { |this| gst::info!(CAT, imp: this, "{msg}") ); - ws_sink.send(WsMessage::Close(None)).await?; ws_sink.close().await?; Ok::<(), Error>(())