Fix websocket connection issues - #10191
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the WebSocket subscription stack (both client and server) against abnormal disconnects, malformed protocol frames, slow handshakes, and close-handshake edge cases, with expanded test coverage to prevent regressions.
Changes:
- Client: ensure handshake failures tear down the receive pipeline; surface abnormal connection loss (no close frame) as
SocketClosedExceptionwith reason1006; improve cancellation behavior so pending reads complete. - Server: enforce correct close-handshake semantics (answer
CloseReceived), truncate overlong close reasons safely, handle invalid JSON as protocol errors, and distinguish init-received vs fully-connected to avoid spurious init timeouts. - Tests: add/adjust protocol and client tests for the above behaviors (abnormal closures, invalid messages, init timeout vs slow connect, keep-alive behavior, and error-vs-complete sequencing).
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/HotChocolate/AspNetCore/test/Transport.Sockets.Client.Tests/GraphQLOverWebSocket/WebSocketClientProtocolTests.cs | Adds client tests for abnormal closure (1006), cancellation completion, invalid server frames, and clean disposal behavior. |
| src/HotChocolate/AspNetCore/test/Transport.Sockets.Client.Tests/GraphQLOverWebSocket/WebSocketClientBatchTests.cs | Updates batch execution tests to expect SocketClosedException on abnormal abort. |
| src/HotChocolate/AspNetCore/test/AspNetCore.Tests/Subscriptions/OperationManagerTests.cs | Adds regression test for stale session completion not disposing successor session with reused IDs. |
| src/HotChocolate/AspNetCore/test/AspNetCore.Tests/Subscriptions/GraphQLOverWebSocket/WebSocketProtocolTests.cs | Updates/extends server protocol tests for forbidden close code, malformed JSON handling, init timeout semantics, and close-frame behavior. |
| src/HotChocolate/AspNetCore/test/AspNetCore.Tests/Subscriptions/Apollo/WebSocketProtocolTests.cs | Adds Apollo protocol tests for long reject messages, terminate/on-close behavior, and keep-alive no-op semantics. |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/SocketClient.cs | Ensures handshake failures dispose the client; surfaces abnormal completion as 1006 unless client initiated teardown. |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/IDataMessage.cs | Makes data messages disposable to enable pooled-buffer cleanup when dropped/drained. |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/GraphQLOverWebSocket/Messages/NextMessage.cs | Implements Dispose() forwarding to OperationResult.Dispose(). |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/GraphQLOverWebSocket/Messages/ErrorMessage.cs | Implements Dispose() forwarding to OperationResult.Dispose(). |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/GraphQLOverWebSocket/Messages/CompleteMessage.cs | Adds no-op Dispose() and tightens id parsing to top-level "id" string. |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/GraphQLOverWebSocket/GraphQLOverWebSocketProtocolHandler.cs | Makes protocol parsing failures non-fatal to the pipeline by converting to 4400 + socket close; improves cancellation completion. |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/DataMessageObserver.cs | Disposes dropped/drained messages to avoid leaking pooled buffers when a stream is disposed or completed early. |
| src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/ConnectionMessageObserver.cs | Improves handshake observer semantics to report close reason (including abnormal 1006) and properly dispose cancellation registration. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/WebSocketSession.cs | Ensures server sends answering close frame when client half-closes (CloseReceived). |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/WebSocketConnection.cs | Adds RequiresClose tracking, correct CloseOutputAsync behavior for CloseReceived, and safe UTF-8 close-reason truncation. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/Protocols/GraphQLOverWebSocket/GraphQLOverWebSocketProtocolHandler.cs | Handles invalid JSON by closing with protocol error; tracks init-received; disposes requests correctly on failure paths. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/Protocols/GraphQLOverWebSocket/ConnectionExtensions.cs | Changes connection-refused close to forbidden (4403) and adds invalid-message close helper. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/Protocols/GraphQLOverWebSocket/CloseReasons.cs | Adds Forbidden close reason (4403). |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/Protocols/Apollo/ApolloSubscriptionProtocolHandler.cs | Treats client keep-alive as no-op; tracks init-received for init timeout semantics. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/PingPongJob.cs | Uses init-received flag (not fully connected) for initialization timeout decision. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/OperationSession.cs | Prevents sending complete after an error terminal message. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/OperationManager.cs | Prevents stale completion events from disposing a successor session with the same id; adds lock coverage in Dispose. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/ISocketConnection.cs | Makes IsConnected get-only and introduces ConnectionInitReceived to separate init-timeout from acceptance timing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/OperationManager.cs:182
- OperationManager.Dispose() clears _subs without disposing the active IOperationSession instances. With the new CompleteSession guard, sessions that complete after Dispose() will no longer be found in _subs and therefore will never be disposed, leaving their CancellationTokenSource and other resources undisposed.
{
_cts.Cancel();
_cts.Dispose();
_lock.EnterWriteLock();
try
{
_subs.Clear();
}
finally
{
_lock.ExitWriteLock();
}
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/Protocols/GraphQLOverWebSocket/ConnectionExtensions.cs:68
- CloseConnectionRefusedAsync is a public extension method, and changing its signature (adding the required message parameter) is a breaking API change for any external callers. Consider keeping the old overload and forwarding it to the new one so existing consumers continue to compile.
public static ValueTask CloseConnectionRefusedAsync(
this ISocketConnection connection,
string message,
CancellationToken cancellationToken)
=> connection.CloseAsync(
message,
CloseReasons.Forbidden,
cancellationToken);
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Subscriptions/ISocketConnection.cs:40
- ISocketConnection.IsConnected changed from get/set to get-only, and a new ConnectionInitReceived property was added. Because ISocketConnection is public, these changes are breaking for any external implementers of the interface. If external implementations are supported, consider preserving the original setter (and potentially adding a setter for ConnectionInitReceived) or introducing a separate internal interface for server-owned mutability while keeping the public interface stable.
/// <summary>
/// Specifies if the connection is connected to a client.
/// </summary>
bool IsConnected { get; }
/// <summary>
/// Specifies if a connection initialization message has been received from the client.
/// This is set as soon as the message arrives, before the connection is accepted, so that
/// the initialization timeout only reflects whether the client sent the message in time and
/// not how long the acceptance (for example authentication) takes.
/// </summary>
bool ConnectionInitReceived { get; }
Contributor
Patch coverage85.8% of changed lines covered (175/204) Uncovered changed lines (JSON){
"sha": "dc7443a89d54925860a36f8dd54be23ab30a8911",
"files": [
{ "path": "src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/GraphQLOverWebSocket/Messages/ErrorMessage.cs", "ranges": [[22, 22]] },
{ "path": "src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/ConnectionMessageObserver.cs", "ranges": [[39, 39], [41, 42], [44, 47], [49, 53], [55, 55], [57, 62]] },
{ "path": "src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/Protocols/GraphQLOverWebSocket/GraphQLOverWebSocketProtocolHandler.cs", "ranges": [[92, 95], [124, 124], [128, 128], [180, 180], [183, 183]] },
{ "path": "src/HotChocolate/AspNetCore/src/Transport.Sockets.Client/SocketClient.cs", "ranges": [[117, 117]] }
]
}Project coverage: 54.4% (242198/445521 lines) |
This was referenced Aug 9, 2026
Merged
Open
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.