Feature/networking issues#137
Open
engbergandreas wants to merge 8 commits into
Open
Conversation
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.
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
engbergandreas
commented
Jul 3, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses WebSocket networking edge cases by ensuring protocol-level frames generated by websocketpp (Ping/Pong/Close handshakes) are actually written back to the peer, and by proactively closing the underlying TCP socket on WebSocket close to avoid lingering connections.
Changes:
- Drain websocketpp’s protocol-generated output after
read_someand write it back to the TCP socket. - Introduce
TcpSocket::closeConnection()as a non-blocking close primitive and use it fromWebSocket::onClose. - Add API documentation for
closeConnection()in the TcpSocket header.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/io/socket/websocket.cpp | Sends websocketpp protocol output after read_some; closes underlying TCP connection on close. |
| src/io/socket/tcpsocket.cpp | Adds TcpSocket::closeConnection() to stop I/O loops and close the socket without disconnect()’s join behavior. |
| include/ghoul/io/socket/tcpsocket.h | Declares and documents the new closeConnection() API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexanderbock
approved these changes
Jul 6, 2026
Comment on lines
+456
to
+466
| bool TcpSocket::waitForOutputQueueDrained(std::chrono::milliseconds timeout) { | ||
| std::unique_lock lock(_outputQueueMutex); | ||
| return _outputNotifier.wait_for( | ||
| lock, | ||
| timeout, | ||
| [this]() { | ||
| return _outputQueue.empty() || _shouldStopThreads || | ||
| (!_isConnected && !_isConnecting); | ||
| } | ||
| ); | ||
| } |
Comment on lines
+351
to
+353
| _outputQueue.erase(_outputQueue.begin(), _outputQueue.begin() + nSentBytes); | ||
| } | ||
| _outputNotifier.notify_all(); // Let anyone waiting on drainage know |
WeirdRubberDuck
requested changes
Jul 17, 2026
Contributor
There was a problem hiding this comment.
Noticed an issue when trying: Closing the Showcomposer while OpenSpace is running now leads to a mutex-related crash
It does not happen when opening the WebUI in a browser and closing it, or the ui.openspaceproject.com pages
Did all my tests using the empty profile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes some networking issues where we did not previously send back a protocol response when needed, for example when a WebSocket disconnects.
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 has been terminated until 10-20 seconds later, when an internal ping fails.
Also fixes an additional issue: possible lingering sockets on our end if the handshake/tear-up is not complete by the peer.
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 correct 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 regardless after receiving and sending the close frame