Fix custom Websocket for IPv6 connection#1153
Conversation
manage StreamWriter state instead of deleting and recreating it every time
subclassing it and overriding `run` method makes `quit()` useless
some mapgen maps can have weird previews which are 'invisible'
server uses 128 in order to not break SQL inserts
there are no previews of them on the content server and they spam logs with 404s
📝 WalkthroughWalkthroughRefactors websocket IO to a QObject-driven reader on a dedicated thread, removes SocketWriter, moves Websocket to compose reader/reader_thread and centralizes error handling; makes StreamWriter reusable with optional GameUrl and lifecycle helpers; small UI and replay-tracking control-flow tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant WS as Websocket
participant RT as ReaderThread
participant SR as SocketReader
participant CC as ClientConnection
WS->>RT: create/start reader_thread
WS->>SR: instantiate SocketReader(conn=None)
WS->>RT: move SR to reader_thread
WS->>SR: connect(_start_read → read)
WS->>SR: connect(message_received → binaryMessageReceived)
WS->>SR: connect(error → handle_error)
WS->>WS: open() -> set self.connection
WS->>SR: set_connection(self.connection)
WS->>WS: emit _start_read
RT->>SR: invoke read()
SR->>CC: read data loop
alt message received
SR->>WS: emit message_received(data)
else error
SR->>WS: emit error(exception)
WS->>WS: handle_error(exception)
end
sequenceDiagram
participant LRS as LiveReplayStreamer
participant SW as StreamWriter
participant API as ReplayAPI
participant NP as NamedPipe/Server
LRS->>SW: instantiate StreamWriter(gurl=None)
LRS->>LRS: wire SW signals once
LRS->>LRS: start_live_replay(gurl)
LRS->>SW: set_game_url(gurl)
LRS->>API: request replay access
alt API success
API->>LRS: return access_url
LRS->>SW: connect_to_server(access_url)
SW->>NP: establish named-pipe
SW->>LRS: emit writer_ready
LRS->>LRS: on_writer_ready() -> start replay
else API error
API->>LRS: error
LRS->>LRS: on_api_error() -> show warning
end
Note over SW: reuse via reset() and set_game_url()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fa/replaylivestreamer.py (1)
102-111:⚠️ Potential issue | 🟡 MinorMissing ERROR_PIPE_CONNECTED success case.
Per previous feedback on this file: when using
win32pipe.ConnectNamedPipe, both a return value of0andERROR_PIPE_CONNECTED(536870394 or winerror 535) should be treated as success. The latter occurs if a client connected betweenCreateNamedPipeandConnectNamedPipe.🛠️ Proposed fix
+ SYNCHRONOUS_SUCCESS = 0 + ERROR_PIPE_CONNECTED = 535 pipe_connected = cast(int, win32pipe.ConnectNamedPipe(pipe, None)) - if pipe_connected != 0: + if pipe_connected not in (SYNCHRONOUS_SUCCESS, ERROR_PIPE_CONNECTED): self._logger.warning(Based on learnings: "In src/fa/replaylivestreamer.py, when using pywin32's win32pipe.ConnectNamedPipe, treat both a return value of 0 (SYNCHRONOUS_SUCCESS) and ERROR_PIPE_CONNECTED as success."
🤖 Fix all issues with AI agents
In `@src/client/workaround_websocket.py`:
- Around line 110-120: The handle_error flow currently emits a NetworkError even
when close() was called intentionally; add a boolean flag (e.g.
self._closed_intentionally) on the connection class, set it True at the start of
close() and False after fully closed, and update handle_error(self, error) to
skip emitting errorOccurred and logging the error when that flag is True (but
still call close() to clean up). Ensure the flag is reset (False) after cleanup
so subsequent real errors are reported.
🧹 Nitpick comments (1)
src/client/workaround_websocket.py (1)
57-63: Thread lifecycle may need cleanup on destruction.The
reader_threadis started but never explicitly stopped inclose(). While this allows thread reuse across connections, ensure the thread is properly terminated when theWebsocketobject is destroyed (e.g., in a destructor or cleanup method) to avoid resource leaks.💡 Consider adding cleanup
def __del__(self) -> None: if self.reader_thread.isRunning(): self.reader_thread.quit() self.reader_thread.wait()Or better, provide an explicit
shutdown()method to be called during application cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/workaround_websocket.py (1)
73-83:⚠️ Potential issue | 🟠 MajorMissing early return after connection failure causes double error handling.
When all addresses fail to connect,
errorOccurredis emitted but execution continues. The caller (open()) proceeds with an unconnected socket, leading to thewebsockets.connect()call failing with a separate exception. This results in inconsistent error handling.🐛 Proposed fix
else: self.errorOccurred.emit(QAbstractSocket.SocketError.NetworkError) + returnThen update
open()to handle this:def open(self, url: QUrl) -> None: self.sock_state = State.CONNECTING self.connect() - assert self.socket is not None + if self.socket is None or not self._is_socket_connected(): + return # Error already emitted by connect()Or alternatively, raise an exception from
connect()when all attempts fail, and catch it inopen().
🧹 Nitpick comments (1)
src/client/workaround_websocket.py (1)
56-62: Consider adding thread cleanup to prevent hangs on shutdown.The
reader_threadis created and started but never explicitly stopped or cleaned up. If theWebsocketobject is destroyed while the thread is running, this could cause the application to hang on exit or leak resources.♻️ Proposed cleanup pattern
Add a cleanup method or destructor:
def cleanup(self) -> None: if self.reader_thread.isRunning(): self.reader_thread.quit() self.reader_thread.wait()Alternatively, connect the thread's cleanup to a parent object's destruction or call it explicitly in
close()when the connection won't be reused.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/_clientwindow.py (1)
1814-1828:⚠️ Potential issue | 🟠 MajorRefresh
GameSessionwhen the logged-in user changes.With the new condition, an existing
GameSessionis reused even ifplayer_id/player_loginno longer match the current login. That can happen on logout/login flows and would keep stale identity in the session. Consider recreating (or updating) the session when the user changes to avoid misattribution during game launch/restore.✅ Suggested adjustment
- if self.game_session is None: + if ( + self.game_session is None + or self.game_session.player_id != self.id + or self.game_session.player_login != self.login + ): self.game_session = GameSession( player_id=self.id, player_login=self.login, )
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.