#930 Try to close WebSocket destination when state is Open or CloseReceived #2091
Conversation
raman-m
added
bug
|
Zhannur, thank you for the pull request! Firstly, have you managed to reproduce the issue through tests? P.S.Please, update your develop branch in forked repo, and pay attention I've rebased the feature branch, so you need to re-checkout the branch locally. |
|
Hello, is this branch only missing tests to fix the bug? Can we contribute and if so, what would be the best way to do it? (Fork the branch on our side?) |
|
@PaulARoy Hi Paul!
Yes, it is. The lack of tests breaches our Dev Process (refer to point 5). I cannot proceed with a code review without tests, as we cannot be certain that the bug fix is effective.
The optimal approach is to pair-program with the author @hogwartsdeveloper to gain access to his forked repository. He will need to add you as a collaborator to his repository. If this is not feasible, please inform me again. We should wait a few weeks for him to add you as a collaborator. |
|
@hogwartsdeveloper Hello Zhannur! Do you plan to complete your PR with tests? P.S. Your PRs have been moved to the September'24 milestone. |
raman-m
added
Spring'25
raman-m
force-pushed
the
develop
branch
7 times, most recently
from
b0cdbd6 to
fa54613
Compare
raman-m
force-pushed
the
issues-930
branch
7 times, most recently
from
3e7aa1a to
99b8f68
Compare
Bug 930 manual tests
There was a problem hiding this comment.
Ready for delivery ✅
- Code reviews ✔️
- Unit testing ✔️
- Acceptance testing ✔️
- Manual testing ☑️ Added draft version of the Bug0930.html webpage which is useful for JavaScript
WebSocketconnection testing. - Docs ☑️ Updates are not required due to the nature of the bug fix.
TODO
Manual testing has revealed more intricate issues in the middleware and Websockets feature when the clients are web browsers.
| protected virtual Task TryCloseOutputAsync(WebSocket webSocket, WebSocketCloseStatus closeStatus, string statusDescription, CancellationToken cancellation) | ||
| => (webSocket.State == WebSocketState.Open || webSocket.State == WebSocketState.CloseReceived) | ||
| ? webSocket.CloseOutputAsync(closeStatus, statusDescription, cancellation) | ||
| : Task.CompletedTask; |
| // DON'T THROW, NEVER! Just log the warning... | ||
| // The logging level has been decreased from level 4 (Error) to level 3 (Warning) due to the high number of disconnecting events for sensitive WebSocket connections in unstable networks. | ||
| Logger.LogWarning(() => $"{nameof(WebSocketException)} when {nameof(e.WebSocketErrorCode)} is {e.WebSocketErrorCode}"); | ||
| return; // swallow the error |
There was a problem hiding this comment.
We no longer throw WebSocketException objects because open WebSocket connections are long-running tasks that run alongside regular short-term HTTP tasks. This should improve Ocelot Core stability, which is crucial for containerized environments.
| return; | ||
| } | ||
|
|
||
| await destination.SendAsync(new ArraySegment<byte>(buffer, 0, result.Count), result.MessageType, result.EndOfMessage, cancellationToken); | ||
| if (destination.State == WebSocketState.Open) |
There was a problem hiding this comment.
This if-statement should, in fact, be incorporated as a condition within the while loop.
| @@ -118,13 +107,13 @@ private async Task Proxy(HttpContext context, DownstreamRequest request, Downstr | |||
| client.Options.AddSubProtocol(protocol); | |||
| } | |||
|
|
|||
| foreach (var headerEntry in context.Request.Headers) | |||
| foreach (var header in context.Request.Headers) | |||
There was a problem hiding this comment.
Conducting a comprehensive review is necessary...
| using var server = await context.WebSockets.AcceptWebSocketAsync(client.SubProtocol); | ||
| await Task.WhenAll( | ||
| PumpAsync(client.ToWebSocket(), server, DefaultWebSocketBufferSize, context.RequestAborted), | ||
| PumpAsync(server, client.ToWebSocket(), DefaultWebSocketBufferSize, context.RequestAborted)); |
There was a problem hiding this comment.
After accepting, the WebSocket connection should be converted into a "tunnel" object and stored in shared storage for all connections. Currently, both pumping tasks are long-running, blocking the first CONNECT-request in the middleware. In the logs, this appears as a hung thread, waiting for both tasks to complete or an exception to be generated.
| [Theory] | ||
| [Trait("Bug", "930")] | ||
| [Trait("PR", "2091")] // https://github.com/ThreeMammals/Ocelot/pull/2091 | ||
| [InlineData("ws", "corefx-net-http11.azurewebsites.net", 80, "/WebSocket/EchoWebSocket.ashx")] // https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/websockets#differences-in-http11-and-http2-websockets | ||
| [InlineData("wss", "echo.websocket.org", 443, "/")] // https://websocket.org/tools/websocket-echo-server/ | ||
| [InlineData("wss", "ws.postman-echo.com", 443, "/raw")] // https://blog.postman.com/introducing-postman-websocket-echo-service/ | ||
| public async Task Http11Client_ConnectionClosedPrematurely_ShouldCloseSocketsWithoutExceptions(string scheme, string host, int port, string path) |
There was a problem hiding this comment.
These are the main acceptance tests, but they only verify a single echo cycle. Therefore, a standard test with multiple echo cycles is necessary, though not specifically for bug fix of #930.
| [Fact] | ||
| [Trait("Bug", "930")] | ||
| [Trait("PR", "2091")] // https://github.com/ThreeMammals/Ocelot/pull/2091 | ||
| public async Task PumpAsync_OperationCanceledException_ClosedDestinationSocket() |
There was a problem hiding this comment.
These, along with the ones below, are the primary unit tests.
… actions: adding DNS records and installing certificates
Fixes #930
Aborted) for this operation. Valid states are:Open,CloseReceived#930Proposed Changes