From 03bd15309d7e318dd29f6799eddb92588f81b587 Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Fri, 3 Jul 2026 13:40:45 +0200 Subject: [PATCH 1/8] Ensure protocol-level WebSocket responses (Ping/Pong/Close etc) Fixes an issue where protocol-level websocket responses were not sent back to the connected peer, which could leave the peer waiting indefinitely for a response. For example, when using the OpenSpace API to disconnect - the api would not be notified that the connection is terminated until 10-20 seconds later when an internal ping fails. --- src/io/socket/websocket.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/io/socket/websocket.cpp b/src/io/socket/websocket.cpp index 30d3db74..96c95bc6 100644 --- a/src/io/socket/websocket.cpp +++ b/src/io/socket/websocket.cpp @@ -59,6 +59,16 @@ WebSocket::WebSocket(std::unique_ptr socket, _tcpSocket->interceptInput( [this](const char* data, size_t nBytes) { _socketConnection->read_some(data, nBytes); + + // `read_some` can cause the websocketpp to generate protocol-level output + // e.g., a Close frame response, or a Pong reply to a Ping. We need to send + // those responses back to the client so they are not kept waiting forever + const std::string output = _outputStream.str(); + if (!output.empty()) { + _tcpSocket->put(output.c_str(), output.size()); + _outputStream.str(""); + } + _inputNotifier.notify_one(); } ); From ffaa6fe48838b1b7c83c933b294b64ff63e9ed87 Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Fri, 3 Jul 2026 14:12:47 +0200 Subject: [PATCH 2/8] Ensure socket closes even if handshake close is not complete from peer There was an issue where if the peer sends a Websocket close frame - ghoul would respond with the corect handshake (previous fix) but if the peer does not properly close the TCP socket this could leave the OpenSpace socket connection open indefinitely(or until shutdown). This fix removes the socket regardles after receiving and sending the close frame --- include/ghoul/io/socket/tcpsocket.h | 7 +++++++ src/io/socket/tcpsocket.cpp | 7 +++++++ src/io/socket/websocket.cpp | 1 + 3 files changed, 15 insertions(+) diff --git a/include/ghoul/io/socket/tcpsocket.h b/include/ghoul/io/socket/tcpsocket.h index 90474293..49ef3fdd 100644 --- a/include/ghoul/io/socket/tcpsocket.h +++ b/include/ghoul/io/socket/tcpsocket.h @@ -73,6 +73,13 @@ class TcpSocket : public Socket { void interceptInput(InputInterceptor interceptor); void uninterceptInput(); + /** + * Non-blocking counterpart to disconnect(), stops the I/O loops and closes the + * socket, but does not join the threads. Safe tocall from within those threads + * themselves (unlike disconnect()) + */ + void closeConnection(); + // Methods for binary communication template bool get(T* buffer, size_t nItems = 1); diff --git a/src/io/socket/tcpsocket.cpp b/src/io/socket/tcpsocket.cpp index 2322fd33..e83a29c1 100644 --- a/src/io/socket/tcpsocket.cpp +++ b/src/io/socket/tcpsocket.cpp @@ -445,6 +445,13 @@ void TcpSocket::uninterceptInput() { _inputInterceptor = nullptr; } +void TcpSocket::closeConnection() { + _shouldStopThreads = true; + closeSocket(); + _inputNotifier.notify_all(); + _outputNotifier.notify_all(); +} + bool TcpSocket::getBytes(char* buffer, size_t nItems) { waitForInput(nItems); if (_shouldStopThreads || (!_isConnected && !_isConnecting)) { diff --git a/src/io/socket/websocket.cpp b/src/io/socket/websocket.cpp index 96c95bc6..76433f61 100644 --- a/src/io/socket/websocket.cpp +++ b/src/io/socket/websocket.cpp @@ -162,6 +162,7 @@ void WebSocket::onClose(const websocketpp::connection_hdl& hdl) { const std::unique_lock lock(_connectionHandlesMutex); _connectionHandles.erase(hdl); _inputNotifier.notify_one(); + _tcpSocket->closeConnection(); } } // namespace ghoul::io From 5d65b137ea367abf58d8069d70f0336153889687 Mon Sep 17 00:00:00 2001 From: Andreas Engberg <48772850+engbergandreas@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:15:06 +0200 Subject: [PATCH 3/8] Apply suggestion from @engbergandreas --- include/ghoul/io/socket/tcpsocket.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/ghoul/io/socket/tcpsocket.h b/include/ghoul/io/socket/tcpsocket.h index 49ef3fdd..dcb97c4f 100644 --- a/include/ghoul/io/socket/tcpsocket.h +++ b/include/ghoul/io/socket/tcpsocket.h @@ -75,7 +75,7 @@ class TcpSocket : public Socket { /** * Non-blocking counterpart to disconnect(), stops the I/O loops and closes the - * socket, but does not join the threads. Safe tocall from within those threads + * socket, but does not join the threads. Safe tocall from within those threads * themselves (unlike disconnect()) */ void closeConnection(); From 97c60c3968cee82e1cfc14e88a13f8bbc8d85d7a Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Fri, 10 Jul 2026 12:14:04 +0200 Subject: [PATCH 4/8] Address multi-thread issues where we got malformed data due to race conditions --- src/io/socket/websocket.cpp | 48 +++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/src/io/socket/websocket.cpp b/src/io/socket/websocket.cpp index 76433f61..83862666 100644 --- a/src/io/socket/websocket.cpp +++ b/src/io/socket/websocket.cpp @@ -58,17 +58,29 @@ WebSocket::WebSocket(std::unique_ptr socket, _tcpSocket->interceptInput( [this](const char* data, size_t nBytes) { - _socketConnection->read_some(data, nBytes); + std::string output; + { + // Guards _socketConnection/_outputStream against putMessage() on other + // threads. onOpen/onMessage/onClose run nested inside read_some(), so + // they must not lock _outputStreamMutex themselves + const std::unique_lock lock(_outputStreamMutex); + _socketConnection->read_some(data, nBytes); + + + // `read_some` can cause the websocketpp to generate protocol-level output + // e.g., a Close frame response, or a Pong reply to a Ping. Flush the + // responses so the client isn't left waiting + output = _outputStream.str(); + if (!output.empty()) { + _outputStream.str(""); + _outputStream.clear(); + } + } - // `read_some` can cause the websocketpp to generate protocol-level output - // e.g., a Close frame response, or a Pong reply to a Ping. We need to send - // those responses back to the client so they are not kept waiting forever - const std::string output = _outputStream.str(); + // Write outside the lock, since TcpSocket::put has its own internal locking if (!output.empty()) { _tcpSocket->put(output.c_str(), output.size()); - _outputStream.str(""); } - _inputNotifier.notify_one(); } ); @@ -76,6 +88,7 @@ WebSocket::WebSocket(std::unique_ptr socket, WebSocket::~WebSocket() { LDEBUG("Destroying socket connection"); + _tcpSocket->uninterceptInput(); _socketConnection->eof(); _tcpSocket = nullptr; } @@ -115,9 +128,22 @@ bool WebSocket::getMessage(std::string& message) { } bool WebSocket::putMessage(const std::string& message) { - _socketConnection->send(message); - _tcpSocket->put(_outputStream.str().c_str(), _outputStream.str().size()); - _outputStream.str(""); + std::string output; + + { + const std::unique_lock lock(_outputStreamMutex); + _socketConnection->send(message); + output = _outputStream.str(); + if (!output.empty()) { + _outputStream.str(""); + _outputStream.clear(); + } + + } + + if (!output.empty()) { + _tcpSocket->put(output.c_str(), output.size()); + } return true; } @@ -149,8 +175,6 @@ void WebSocket::onOpen(const websocketpp::connection_hdl& hdl) { )); const std::unique_lock lock(_connectionHandlesMutex); _connectionHandles.insert(hdl); - _tcpSocket->put(_outputStream.str().c_str(), _outputStream.str().size()); - _outputStream.str(""); } void WebSocket::onClose(const websocketpp::connection_hdl& hdl) { From a8085544ca1b63f3634884c6156b51e71b0d310d Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Fri, 10 Jul 2026 12:45:07 +0200 Subject: [PATCH 5/8] fix typo --- include/ghoul/io/socket/tcpsocket.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/ghoul/io/socket/tcpsocket.h b/include/ghoul/io/socket/tcpsocket.h index dcb97c4f..099ed46d 100644 --- a/include/ghoul/io/socket/tcpsocket.h +++ b/include/ghoul/io/socket/tcpsocket.h @@ -75,7 +75,7 @@ class TcpSocket : public Socket { /** * Non-blocking counterpart to disconnect(), stops the I/O loops and closes the - * socket, but does not join the threads. Safe tocall from within those threads + * socket, but does not join the threads. Safe to call from within those threads * themselves (unlike disconnect()) */ void closeConnection(); From 7451a132e4e9802169e2f814c768bc1fd915839e Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Mon, 13 Jul 2026 10:21:58 +0200 Subject: [PATCH 6/8] defer closing the socket until after we've sent the close frame --- include/ghoul/io/socket/tcpsocket.h | 1 + include/ghoul/io/socket/websocket.h | 1 + src/io/socket/tcpsocket.cpp | 12 ++++++++++++ src/io/socket/websocket.cpp | 6 +++++- 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/include/ghoul/io/socket/tcpsocket.h b/include/ghoul/io/socket/tcpsocket.h index 099ed46d..4dbb23dc 100644 --- a/include/ghoul/io/socket/tcpsocket.h +++ b/include/ghoul/io/socket/tcpsocket.h @@ -79,6 +79,7 @@ class TcpSocket : public Socket { * themselves (unlike disconnect()) */ void closeConnection(); + void waitForOuputQueueDrained(); // Methods for binary communication template diff --git a/include/ghoul/io/socket/websocket.h b/include/ghoul/io/socket/websocket.h index d105b1af..500b76d7 100644 --- a/include/ghoul/io/socket/websocket.h +++ b/include/ghoul/io/socket/websocket.h @@ -115,6 +115,7 @@ class WebSocket : public Socket { std::condition_variable _inputNotifier; std::unique_ptr _tcpSocket; + bool _isMarkedForClosing = false; }; } // namespace ghoul::io diff --git a/src/io/socket/tcpsocket.cpp b/src/io/socket/tcpsocket.cpp index e83a29c1..270e56d8 100644 --- a/src/io/socket/tcpsocket.cpp +++ b/src/io/socket/tcpsocket.cpp @@ -350,6 +350,7 @@ void TcpSocket::streamOutput() { } _outputQueue.erase(_outputQueue.begin(), _outputQueue.begin() + nBytesToSend); } + _outputNotifier.notify_all(); // Let anyone waiting on drainage know } } @@ -452,6 +453,17 @@ void TcpSocket::closeConnection() { _outputNotifier.notify_all(); } +void TcpSocket::waitForOuputQueueDrained() { + std::unique_lock lock(_outputQueueMutex); + _outputNotifier.wait( + lock, + [this]() { + return _outputQueue.empty() || _shouldStopThreads || + (!_isConnected && !_isConnecting); + } + ); +} + bool TcpSocket::getBytes(char* buffer, size_t nItems) { waitForInput(nItems); if (_shouldStopThreads || (!_isConnected && !_isConnecting)) { diff --git a/src/io/socket/websocket.cpp b/src/io/socket/websocket.cpp index 83862666..309b4913 100644 --- a/src/io/socket/websocket.cpp +++ b/src/io/socket/websocket.cpp @@ -81,6 +81,10 @@ WebSocket::WebSocket(std::unique_ptr socket, if (!output.empty()) { _tcpSocket->put(output.c_str(), output.size()); } + if (_isMarkedForClosing) { + _tcpSocket->waitForOuputQueueDrained(); + _tcpSocket->closeConnection(); + } _inputNotifier.notify_one(); } ); @@ -186,7 +190,7 @@ void WebSocket::onClose(const websocketpp::connection_hdl& hdl) { const std::unique_lock lock(_connectionHandlesMutex); _connectionHandles.erase(hdl); _inputNotifier.notify_one(); - _tcpSocket->closeConnection(); + _isMarkedForClosing = true; } } // namespace ghoul::io From e9838a5bd916455e0a58231ec8a2a007afc2dcad Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Mon, 13 Jul 2026 10:39:41 +0200 Subject: [PATCH 7/8] add timeout waiting for output queue drainage --- include/ghoul/io/socket/tcpsocket.h | 2 +- src/io/socket/tcpsocket.cpp | 5 +++-- src/io/socket/websocket.cpp | 6 +++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/ghoul/io/socket/tcpsocket.h b/include/ghoul/io/socket/tcpsocket.h index 4dbb23dc..87ad065c 100644 --- a/include/ghoul/io/socket/tcpsocket.h +++ b/include/ghoul/io/socket/tcpsocket.h @@ -79,7 +79,7 @@ class TcpSocket : public Socket { * themselves (unlike disconnect()) */ void closeConnection(); - void waitForOuputQueueDrained(); + bool waitForOutputQueueDrained(std::chrono::milliseconds timeout); // Methods for binary communication template diff --git a/src/io/socket/tcpsocket.cpp b/src/io/socket/tcpsocket.cpp index 270e56d8..7a5392b2 100644 --- a/src/io/socket/tcpsocket.cpp +++ b/src/io/socket/tcpsocket.cpp @@ -453,10 +453,11 @@ void TcpSocket::closeConnection() { _outputNotifier.notify_all(); } -void TcpSocket::waitForOuputQueueDrained() { +bool TcpSocket::waitForOutputQueueDrained(std::chrono::milliseconds timeout) { std::unique_lock lock(_outputQueueMutex); - _outputNotifier.wait( + return _outputNotifier.wait_for( lock, + timeout, [this]() { return _outputQueue.empty() || _shouldStopThreads || (!_isConnected && !_isConnecting); diff --git a/src/io/socket/websocket.cpp b/src/io/socket/websocket.cpp index 309b4913..5ba8cc7c 100644 --- a/src/io/socket/websocket.cpp +++ b/src/io/socket/websocket.cpp @@ -82,7 +82,11 @@ WebSocket::WebSocket(std::unique_ptr socket, _tcpSocket->put(output.c_str(), output.size()); } if (_isMarkedForClosing) { - _tcpSocket->waitForOuputQueueDrained(); + const bool drained = + _tcpSocket->waitForOutputQueueDrained(std::chrono::seconds(3)); + if (!drained) { + LWARNING("Timed out flushing final output before closing socket"); + } _tcpSocket->closeConnection(); } _inputNotifier.notify_one(); From f82064185086011664daf829f37d5684f0fc6729 Mon Sep 17 00:00:00 2001 From: Andreas Engberg Date: Mon, 13 Jul 2026 10:55:36 +0200 Subject: [PATCH 8/8] remove only the sent bytes, treat 0 bytes as an error --- src/io/socket/tcpsocket.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/io/socket/tcpsocket.cpp b/src/io/socket/tcpsocket.cpp index 7a5392b2..0e53b4b6 100644 --- a/src/io/socket/tcpsocket.cpp +++ b/src/io/socket/tcpsocket.cpp @@ -338,7 +338,7 @@ void TcpSocket::streamOutput() { nBytesToSend, 0 ); - auto failed = [](ssize_t nBytes) { return nBytes == ssize_t(-1); }; + auto failed = [](ssize_t nBytes) { return nBytes <= 0; }; #endif // WIN32 if (failed(nSentBytes)) { @@ -348,7 +348,7 @@ void TcpSocket::streamOutput() { _outputNotifier.notify_all(); return; } - _outputQueue.erase(_outputQueue.begin(), _outputQueue.begin() + nBytesToSend); + _outputQueue.erase(_outputQueue.begin(), _outputQueue.begin() + nSentBytes); } _outputNotifier.notify_all(); // Let anyone waiting on drainage know }