feat: expose IsAlive on WebSocketBehavior - #8
Closed
andreion1ca wants to merge 3 commits into
Closed
Conversation
AltTester-Server's handler base re-exposes the session's liveness (AltWebSocketHandler.IsAlive). The vendored server build it currently ships (altserverwebsocket-sharp.dll) has this property, this library does not, so the server cannot compile against it. That was the last API gap keeping AltTester-Server on the vendored build, whose WebSocket.open() still dispatches the queued message via Delegate.BeginInvoke. BeginInvoke throws PlatformNotSupportedException on .NET Core, which kills any connection whose peer speaks before OnOpen returns - AltTester-Server issue sta#181. This library already dispatches via Task.Factory.StartNew under NET_CORE, so moving the server onto it fixes the issue.
receiveRequest parked in TcpListener.AcceptTcpClient. On Unix a blocking accept holds a reference on the listener's socket handle, and TcpListener.Stop() -> Socket.Dispose() spin-waits for that reference without interrupting the accept, so any server that had accepted at least one connection hung forever on shutdown. Poll the listening socket with a 250ms timeout instead and re-check the server state each round, which keeps the accept unblocked and lets the loop retire on its own.
Both server close paths released the connection and only then ran the closing handshake, so the close frame was written to a stream that was already gone. The peer never saw the code and reason and reported an abnormal 1006 close instead - which loses every application close code the server relies on to explain itself (unsupported SDK, duplicate app name, connection limits). Run the handshake first, then release the transport and mark the socket closed.
There was a problem hiding this comment.
Pull request overview
This PR improves server-side WebSocket shutdown behavior and exposes connection-liveness information to service implementations via WebSocketBehavior.
Changes:
- Reorders parts of the server close sequence so the close frame/handshake is attempted before tearing down server transport resources.
- Adds a polling-based accept loop in
WebSocketServerto avoid shutdown deadlocks on Unix and to bound shutdown wait time. - Exposes
IsAliveas a protected property onWebSocketBehaviorfor session implementations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| websocket-sharp/websocket-sharp/ServerWebSocket.cs | Adjusts server close sequencing to perform handshake/close-frame send before resource teardown. |
| websocket-sharp/websocket-sharp/Server/WebSocketServer.cs | Adds a timed poll before AcceptTcpClient() to make shutdown responsive and avoid Unix accept/stop deadlock. |
| websocket-sharp/websocket-sharp/Server/WebSocketBehavior.cs | Adds protected bool IsAlive to allow derived behaviors to check session connection liveness. |
Suppressed comments (1)
websocket-sharp/websocket-sharp/ServerWebSocket.cs:581
SendClosingBytes()disposesreceivingExitedForLaterbeforeReleaseCommonResources(false)clearsreceivingExitedEvent. That leaves a race where the receive loop can still attemptreceivingExitedEvent?.Set()on a disposed event, potentially throwingObjectDisposedException. Deferring disposal until after the field is cleared (or guardingSet()) avoids this.
// Called outside the lock, and before the transport is torn down: the closing frame
// has to reach a live stream, otherwise the peer sees an abnormal disconnect (1006)
// instead of the code and reason we are closing with.
var wasClean = SendClosingBytes();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+456
to
460
| // Called outside the lock, and before the transport is torn down: the closing | ||
| // handshake has to write the close frame to a live stream, otherwise the peer sees an | ||
| // abnormal disconnect (1006) instead of the code and reason we are closing with. | ||
| var wasClean = DoClosingHandshake(); | ||
|
|
Comment on lines
+925
to
+933
| // Poll with a timeout rather than parking in AcceptTcpClient: on Unix a | ||
| // blocking accept holds a reference on the listener's socket handle, and | ||
| // TcpListener.Stop() -> Socket.Dispose() spin-waits for that reference to be | ||
| // released without interrupting the accept. A server that has accepted at | ||
| // least one connection therefore deadlocks on shutdown. Polling keeps the | ||
| // accept unblocked and lets this loop observe the shutdown itself. | ||
| if (!_listener.Server.Poll(_acceptPollTimeout, SelectMode.SelectRead)) | ||
| continue; | ||
|
|
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.
No description provided.