From 4e7f097bf081f13c4f8e374199d63ed877112e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Sim=C3=B5es?= Date: Thu, 24 Sep 2026 15:12:25 +0100 Subject: [PATCH 1/2] Fix potential deadlock in WebSocket timeout checker - Remove Thread.Suspend/Resume from the timeout checker. - Guard the shared connection state with a lock that is never held while sending, waiting or running user code. - Make close, ping and pong state changes atomic, so only one close frame is ever sent. - Make the ping-timeout close non-blocking. - Skip a timer check when the previous one is still running. - Clear the ping flag when the ping can't be queued. - Stop the timer before closing the receive stream. --- WebSockets/ReceiveAndControllThread.cs | 79 +++++----- WebSockets/WebSocket.cs | 193 +++++++++++++++++++++---- 2 files changed, 204 insertions(+), 68 deletions(-) diff --git a/WebSockets/ReceiveAndControllThread.cs b/WebSockets/ReceiveAndControllThread.cs index e658057..d1138c0 100644 --- a/WebSockets/ReceiveAndControllThread.cs +++ b/WebSockets/ReceiveAndControllThread.cs @@ -13,6 +13,7 @@ namespace System.Net.WebSockets internal class ReceiveAndControllThread { private readonly WebSocket _webSocket; + private int _checkingTimeouts = 0; public ReceiveAndControllThread(WebSocket webSocket) { @@ -21,8 +22,8 @@ public ReceiveAndControllThread(WebSocket webSocket) public void WorkerThread() //this thread is always running and thus the best place for controlling ping and other messages { - var timeoutCheckerTimer = new Timer(CheckTimeouts, Thread.CurrentThread, 5000, 5000); - + var timeoutCheckerTimer = new Timer(CheckTimeouts, null, 5000, 5000); + while (!_webSocket.Stopped) { @@ -52,9 +53,9 @@ public ReceiveAndControllThread(WebSocket webSocket) } } - _webSocket.ReceiveStream.Close(); timeoutCheckerTimer.Change(Timeout.Infinite, Timeout.Infinite); timeoutCheckerTimer.Dispose(); + _webSocket.ReceiveStream.Close(); } private void ProcessIncomingMessage() @@ -80,7 +81,7 @@ private void ProcessIncomingMessage() { byte[] buffer = _webSocket.WebSocketReceiver.ReadBuffer(messageFrame.MessageLength, messageFrame.Masks); - _webSocket.LastContactTimeStamp = DateTime.UtcNow; + _webSocket.TouchLastContact(); switch (messageFrame.OpCode) { @@ -95,7 +96,7 @@ private void ProcessIncomingMessage() case OpCode.PongFrame: // received a Pong // checking if content Pong matches Ping is not implemented due to thread safety and memory consumption considerations - _webSocket.Pinging = false; + _webSocket.OnPongReceived(); break; case OpCode.ConnectionCloseFrame: @@ -112,17 +113,14 @@ private void ProcessIncomingMessage() } //connection asked to be closed return answer - if (_webSocket.State != WebSocketFrame.WebSocketState.CloseSent) + if (_webSocket.TryMarkCloseReceived()) { - _webSocket.State = WebSocketFrame.WebSocketState.CloseReceived; - _webSocket.RawClose(WebSocketCloseStatus.NormalClosure, buffer, true); } - //response to connection close we can shut down the socket. - else - { - _webSocket.HardClose(); - } + + // either this is the response to our close, or RawClose lost a race with another close, + // so we can shut down the socket (no-op if already closed) + _webSocket.HardClose(); break; } } @@ -138,7 +136,7 @@ private void ProcessIncomingMessage() { messageFrame.Buffer = _webSocket.WebSocketReceiver.ReadBuffer(messageFrame.MessageLength, messageFrame.Masks); - _webSocket.LastContactTimeStamp = DateTime.UtcNow; + _webSocket.TouchLastContact(); OnNewMessage(messageFrame); } @@ -147,37 +145,41 @@ private void ProcessIncomingMessage() } - private void CheckTimeouts(object thread) + // Runs on the timer thread, concurrently with the receive thread. + // Shared state is accessed through WebSocket helpers that only hold a lock for field access, + // and nothing here blocks, so a receive thread holding a lock can't stall this check. + private void CheckTimeouts(object state) { - var receiveThread = (Thread)thread; - -#pragma warning disable S3889 // OK to use in .NET nanoFramework context - receiveThread.Suspend(); -#pragma warning restore S3889 // Neither "Thread.Resume" nor "Thread.Suspend" should be used + // skip this tick if the previous one is still running + if (Interlocked.CompareExchange(ref _checkingTimeouts, 1, 0) != 0) + { + return; + } try { - //Controlling ping and ControllerMessagesTimeout - if (_webSocket.Pinging - && _webSocket.PingTime.Add(_webSocket.ServerTimeout) < DateTime.UtcNow) + if (_webSocket.Stopped) { - _webSocket.RawClose(WebSocketCloseStatus.PolicyViolation, Encoding.UTF8.GetBytes("Ping timeout"), true); - - Debug.WriteLine($"{_webSocket.RemoteEndPoint} ping timed out"); + return; } - if (_webSocket.State == WebSocketFrame.WebSocketState.CloseSent - && _webSocket.ClosingTime.Add(_webSocket.ServerTimeout) < DateTime.UtcNow) + //Controlling ping and ControllerMessagesTimeout + switch (_webSocket.EvaluateTimeouts(DateTime.UtcNow)) { - _webSocket.HardClose(); - } + case WebSocket.TimeoutAction.PingTimeout: + Debug.WriteLine($"{_webSocket.RemoteEndPoint} ping timed out"); - if (_webSocket.KeepAliveInterval != Timeout.InfiniteTimeSpan - && _webSocket.State != WebSocketFrame.WebSocketState.CloseSent - && !_webSocket.Pinging - && _webSocket.LastContactTimeStamp.Add(_webSocket.KeepAliveInterval) < DateTime.UtcNow) - { - _webSocket.SendPing(); + // don't wait for the close message to be sent, a following check will close the connection + _webSocket.BeginClose(WebSocketCloseStatus.PolicyViolation, Encoding.UTF8.GetBytes("Ping timeout")); + break; + + case WebSocket.TimeoutAction.HardClose: + _webSocket.HardClose(); + break; + + case WebSocket.TimeoutAction.SendPing: + _webSocket.SendPing(); + break; } } catch (Exception ex) @@ -186,10 +188,7 @@ private void CheckTimeouts(object thread) } finally { - // always resume the receive thread, otherwise it will be left suspended forever -#pragma warning disable S3889 // OK to use in .NET nanoFramework context - receiveThread.Resume(); -#pragma warning restore S3889 // Neither "Thread.Resume" nor "Thread.Suspend" should be used + Interlocked.Exchange(ref _checkingTimeouts, 0); } } diff --git a/WebSockets/WebSocket.cs b/WebSockets/WebSocket.cs index ad5fc82..d50ab29 100644 --- a/WebSockets/WebSocket.cs +++ b/WebSockets/WebSocket.cs @@ -28,6 +28,21 @@ public abstract class WebSocket : IDisposable internal WebSocketSender _webSocketSender; private readonly object _syncLock = new object(); private int _hardClosed = 0; + + // protects the connection state fields shared between the receive thread, the timeout checker and the send thread + // never hold it while doing I/O, sleeping, queueing frames or calling user code + private readonly object _stateLock = new object(); + private bool _closeFrameQueued = false; + private bool _hardCloseWhenCloseSent = false; + + internal enum TimeoutAction + { + None, + SendPing, + PingTimeout, + HardClose + } + internal MessageReceivedEventHandler CallbacksMessageReceivedEventHandler; /// @@ -135,6 +150,8 @@ protected void ConnectToStream(NetworkStream stream, bool isServer, Socket socke RemoteEndPoint = (IPEndPoint)socket.RemoteEndPoint; LastContactTimeStamp = DateTime.UtcNow; _hardClosed = 0; + _closeFrameQueued = false; + _hardCloseWhenCloseSent = false; //start server sending and receiving async WebSocketReceiver = new WebSocketReceiver(stream, RemoteEndPoint, this, IsServer, MaxReceiveFrameSize, OnReadError); @@ -228,26 +245,71 @@ public void Close(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.Empty, /// public void Abort() { - State = WebSocketState.Aborted; + lock (_stateLock) + { + State = WebSocketState.Aborted; + } HardClose(); } - // CloseImediately will Send a close message and not await this message. + // CloseImediately will Send a close message and not await this message. internal void RawClose(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.Empty, byte[] buffer = null, bool CloseImmediately = false) { - //send closing message which needs to be awaited for a period - if (!(State == WebSocketState.Open || State == WebSocketState.CloseReceived)) + if (!QueueCloseFrame(closeStatus, buffer)) { //already closing or closed - return; + return; + } + + if (CloseImmediately) + { + // Give it a moment for sending a close message. This will block the thread. + // Wait is bounded because the send thread can be stuck on a dead connection. + int maxWaitMs = (int)ServerTimeout.TotalMilliseconds; + + if (maxWaitMs <= 0) + { + // infinite (or invalid) server timeout, use a sensible default + maxWaitMs = 5000; + } + + int msWaited = 0; + + while (!_webSocketSender.CloseMessageSent + && msWaited < maxWaitMs) + { + msWaited += 50; + Thread.Sleep(50); + } + + HardClose(); } + } - CloseStatus = closeStatus; + // Non blocking close, used by the timeout checker. + // The connection is hard closed by a later timeout check, once the close message is sent or ServerTimeout expires. + internal void BeginClose(WebSocketCloseStatus closeStatus, byte[] buffer) + { + if (QueueCloseFrame(closeStatus, buffer)) + { + lock (_stateLock) + { + _hardCloseWhenCloseSent = true; + } + } + } + + private bool QueueCloseFrame(WebSocketCloseStatus closeStatus, byte[] buffer) + { + if (!TryBeginClose(closeStatus)) + { + return false; + } byte[] sendBuffer = new byte[0]; - if (!(closeStatus == WebSocketCloseStatus.ClosedAbnormally || closeStatus == WebSocketCloseStatus.Empty)) + if (!(closeStatus == WebSocketCloseStatus.ClosedAbnormally || closeStatus == WebSocketCloseStatus.Empty)) { if (buffer == null) { @@ -275,44 +337,116 @@ internal void RawClose(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.E OpCode = OpCode.ConnectionCloseFrame, }); - if (CloseImmediately) + return true; + } + + // Atomically moves to CloseSent, making sure only one close message is ever queued. + private bool TryBeginClose(WebSocketCloseStatus closeStatus) + { + lock (_stateLock) { - State = WebSocketState.CloseSent; + if (_closeFrameQueued + || !(State == WebSocketState.Open || State == WebSocketState.CloseReceived)) + { + return false; + } + + _closeFrameQueued = true; + CloseStatus = closeStatus; + + // set ClosingTime before State so the timeout checker never sees CloseSent with a stale ClosingTime ClosingTime = DateTime.UtcNow; + State = WebSocketState.CloseSent; - // Give it a moment for sending a close message. This will block the thread. - // Wait is bounded because the send thread can be stuck on a dead connection. - int maxWaitMs = (int)ServerTimeout.TotalMilliseconds; + return true; + } + } - if (maxWaitMs <= 0) + // Called when the peer sent a close message. + // Returns true if the close message has to be answered, false if the connection is already closing and can be hard closed. + internal bool TryMarkCloseReceived() + { + lock (_stateLock) + { + if (_closeFrameQueued + || !(State == WebSocketState.Open || State == WebSocketState.CloseReceived)) { - // infinite (or invalid) server timeout, use a sensible default - maxWaitMs = 5000; + return false; } - int msWaited = 0; + State = WebSocketState.CloseReceived; - while (!_webSocketSender.CloseMessageSent - && msWaited < maxWaitMs) + return true; + } + } + + internal void TouchLastContact() + { + lock (_stateLock) + { + LastContactTimeStamp = DateTime.UtcNow; + } + } + + internal void OnPongReceived() + { + lock (_stateLock) + { + Pinging = false; + } + } + + // Decides what the timeout checker has to do. When it returns SendPing the ping is already flagged as in progress. + internal TimeoutAction EvaluateTimeouts(DateTime now) + { + lock (_stateLock) + { + if (State == WebSocketState.CloseSent) { - msWaited += 50; - Thread.Sleep(50); + if ((_hardCloseWhenCloseSent && _webSocketSender.CloseMessageSent) + || ClosingTime.Add(ServerTimeout) < now) + { + return TimeoutAction.HardClose; + } + + return TimeoutAction.None; } - HardClose(); + if (State != WebSocketState.Open) + { + return TimeoutAction.None; + } + + if (Pinging) + { + return PingTime.Add(ServerTimeout) < now ? TimeoutAction.PingTimeout : TimeoutAction.None; + } + + if (KeepAliveInterval != Timeout.InfiniteTimeSpan + && LastContactTimeStamp.Add(KeepAliveInterval) < now) + { + Pinging = true; + PingTime = now; + + return TimeoutAction.SendPing; + } + + return TimeoutAction.None; } } internal void HardClose() { // can be called from several threads (receive, send error, timeout checker) - // lock-free guard: the timeout checker suspends the receive thread, so a lock here could deadlock if (Interlocked.CompareExchange(ref _hardClosed, 1, 0) != 0) { return; } - State = WebSocketState.Closed; + lock (_stateLock) + { + State = WebSocketState.Closed; + } StopReceiving(); _webSocketSender.StopSender(); @@ -376,18 +510,21 @@ private void OnWriteError(object sender, WebSocketWriteErrorEventArgs e) } - //Ping will only commence from this thread because of threading safety. + // Pinging and PingTime are set by EvaluateTimeouts, which decides when a ping is due. internal void SendPing(string pingContent = "hello") { - Pinging = true; - PingTime = DateTime.UtcNow; - - QueueMessageToSend(new SendMessageFrame() + bool queued = QueueMessageToSend(new SendMessageFrame() { Buffer = Encoding.UTF8.GetBytes(pingContent), OpCode = OpCode.PingFrame }); + + if (!queued) + { + // connection is closing, no pong will come back + OnPongReceived(); + } } internal void StopReceiving() From f7c1169482836c88e23230161feae609e7f2d062 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Sim=C3=B5es?= Date: Thu, 24 Sep 2026 15:27:19 +0100 Subject: [PATCH 2/2] Several fixes with Close() - Close() now sends a close frame and ends the connection when the peer replies. - Close(EndpointUnavailable) and WebSocketServer.Stop() block until the frame is written. - Data sent before a normal Close() goes out before the close frame. --- WebSockets/ReceiveAndControllThread.cs | 5 ++ WebSockets/WebSocket.cs | 63 ++++++++++++++------------ WebSockets/WebSocketSender.cs | 28 ++++++++---- 3 files changed, 60 insertions(+), 36 deletions(-) diff --git a/WebSockets/ReceiveAndControllThread.cs b/WebSockets/ReceiveAndControllThread.cs index d1138c0..06ca5d6 100644 --- a/WebSockets/ReceiveAndControllThread.cs +++ b/WebSockets/ReceiveAndControllThread.cs @@ -117,6 +117,11 @@ private void ProcessIncomingMessage() { _webSocket.RawClose(WebSocketCloseStatus.NormalClosure, buffer, true); } + else + { + // our close message can still be queued behind pending messages (simultaneous close) + _webSocket.WaitForCloseMessageSent(); + } // either this is the response to our close, or RawClose lost a race with another close, // so we can shut down the socket (no-op if already closed) diff --git a/WebSockets/WebSocket.cs b/WebSockets/WebSocket.cs index d50ab29..2ff8b44 100644 --- a/WebSockets/WebSocket.cs +++ b/WebSockets/WebSocket.cs @@ -230,14 +230,14 @@ public void Close(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.Empty, if (State != WebSocketState.Open) { //already closing or closed - return; + return; } - State = WebSocketState.CloseSent; - - ClosingTime = DateTime.UtcNow; + bool closeImmediately = closeStatus == WebSocketCloseStatus.EndpointUnavailable; - RawClose(closeStatus, statusDescription == null ? null : Encoding.UTF8.GetBytes(statusDescription), closeStatus == WebSocketCloseStatus.EndpointUnavailable); + // state transition (and ClosingTime) is done atomically by TryBeginClose + // a normal close is sent after the messages already queued, so these aren't lost + RawClose(closeStatus, statusDescription == null ? null : Encoding.UTF8.GetBytes(statusDescription), closeImmediately, !closeImmediately); } /// @@ -254,9 +254,10 @@ public void Abort() } // CloseImediately will Send a close message and not await this message. - internal void RawClose(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.Empty, byte[] buffer = null, bool CloseImmediately = false) + // afterPendingMessages will send the close message after the messages already queued, instead of before them. + internal void RawClose(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.Empty, byte[] buffer = null, bool CloseImmediately = false, bool afterPendingMessages = false) { - if (!QueueCloseFrame(closeStatus, buffer)) + if (!QueueCloseFrame(closeStatus, buffer, afterPendingMessages)) { //already closing or closed return; @@ -264,26 +265,32 @@ internal void RawClose(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.E if (CloseImmediately) { - // Give it a moment for sending a close message. This will block the thread. - // Wait is bounded because the send thread can be stuck on a dead connection. - int maxWaitMs = (int)ServerTimeout.TotalMilliseconds; + WaitForCloseMessageSent(); - if (maxWaitMs <= 0) - { - // infinite (or invalid) server timeout, use a sensible default - maxWaitMs = 5000; - } + HardClose(); + } + } + + // Give it a moment for sending a close message. This will block the thread. + // Wait is bounded because the send thread can be stuck on a dead connection. + // Returns immediately if the connection is already closed. + internal void WaitForCloseMessageSent() + { + int maxWaitMs = (int)ServerTimeout.TotalMilliseconds; - int msWaited = 0; + if (maxWaitMs <= 0) + { + // infinite (or invalid) server timeout, use a sensible default + maxWaitMs = 5000; + } - while (!_webSocketSender.CloseMessageSent - && msWaited < maxWaitMs) - { - msWaited += 50; - Thread.Sleep(50); - } + int msWaited = 0; - HardClose(); + while (!_webSocketSender.CloseMessageSent + && msWaited < maxWaitMs) + { + msWaited += 50; + Thread.Sleep(50); } } @@ -291,7 +298,7 @@ internal void RawClose(WebSocketCloseStatus closeStatus = WebSocketCloseStatus.E // The connection is hard closed by a later timeout check, once the close message is sent or ServerTimeout expires. internal void BeginClose(WebSocketCloseStatus closeStatus, byte[] buffer) { - if (QueueCloseFrame(closeStatus, buffer)) + if (QueueCloseFrame(closeStatus, buffer, false)) { lock (_stateLock) { @@ -300,7 +307,7 @@ internal void BeginClose(WebSocketCloseStatus closeStatus, byte[] buffer) } } - private bool QueueCloseFrame(WebSocketCloseStatus closeStatus, byte[] buffer) + private bool QueueCloseFrame(WebSocketCloseStatus closeStatus, byte[] buffer, bool afterPendingMessages) { if (!TryBeginClose(closeStatus)) { @@ -335,7 +342,7 @@ private bool QueueCloseFrame(WebSocketCloseStatus closeStatus, byte[] buffer) { Buffer = sendBuffer, OpCode = OpCode.ConnectionCloseFrame, - }); + }, afterPendingMessages); return true; } @@ -472,7 +479,7 @@ internal void HardClose() } } - internal bool QueueMessageToSend(SendMessageFrame frame) + internal bool QueueMessageToSend(SendMessageFrame frame, bool afterPendingMessages = false) { // if connection is closing only a close respond can be send. if (State != WebSocketState.Open && frame.OpCode != OpCode.ConnectionCloseFrame) @@ -483,7 +490,7 @@ internal bool QueueMessageToSend(SendMessageFrame frame) frame.EndPoint = RemoteEndPoint; - _webSocketSender.QueueMessage(frame); + _webSocketSender.QueueMessage(frame, afterPendingMessages); return true; } diff --git a/WebSockets/WebSocketSender.cs b/WebSockets/WebSocketSender.cs index 0a0e64c..2d4abf8 100644 --- a/WebSockets/WebSocketSender.cs +++ b/WebSockets/WebSocketSender.cs @@ -49,9 +49,10 @@ internal WebSocketSender(NetworkStream outputStream, bool isServer, WebSocketWri _webSocketWriteErrorCallback = webSocketWriteErrorCallback; } - internal void QueueMessage(SendMessageFrame sendMessage) + // afterPendingMessages queues a control frame with the message frames, so it's sent after the ones already queued + internal void QueueMessage(SendMessageFrame sendMessage, bool afterPendingMessages = false) { - if (sendMessage.IsControllFrame) + if (sendMessage.IsControllFrame && !afterPendingMessages) { lock (_controlMessages.SyncRoot) { @@ -121,6 +122,12 @@ private bool ProcessMessageFrames() // message is not fragmented or fragment is smaller that the buffer size // good to go in a single batch SendFrame(messageFrame); + + if (messageFrame.OpCode == OpCode.ConnectionCloseFrame) + { + // close queued after pending messages + OnCloseFrameSent(); + } } else { @@ -202,18 +209,23 @@ private bool ProcessControlMessages() // no need to keep checking messages stack keepChecking = false; - // close message was sent - CloseMessageSent = true; - - // wait until resources can be released. - _shutdownEvent.WaitOne(); + OnCloseFrameSent(); } itemSend = true; } } - return itemSend; + return itemSend; + } + + private void OnCloseFrameSent() + { + // close message was sent + CloseMessageSent = true; + + // wait until resources can be released. + _shutdownEvent.WaitOne(); }