-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WebSocket Keep-Alive Ping and Timeout (minimal) implementation #105841
Conversation
Note regarding the
|
1 similar comment
Note regarding the
|
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketCreationOptions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There seem to be a related test crash, but it's on OSX so I can't check which test it is coming from.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read-ahead capability will be added separately.
Do we need it?
Presumably most WebSocket users will have pending reads at all times already (I'm guessing we're relying on that with the current approach).
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/NetEventSource.WebSockets.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocketOptions.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketCreationOptions.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM, most of the comments are "stylistical". The only one functional is the exception type depending on timing.
I also noticed, there's a lot of minor product code changes just for the sake of logging. I assume your perf measurements are done with these changes (with logging off obviously) and they do not affect it.
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/LoopbackServer/Http2LoopbackStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
This is important feature for ASP.NET, we should get it in ASAP today (Fri 8/9). I approve. @stephentoub given it is larger change, can you please review it as well post merge? If there is anything critical, let us know. We can address non-critical feedback in follow up PR. |
All CI failures are unrelated known issues (build analysis green) |
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocketOptions.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
Show resolved
Hide resolved
Feedback follow-up in #107662 |
This is a minimal implementation of the API without additional read-ahead logic. In order for the feature to work (to consume Pongs as they arrive in response to the Pings), there needs to be an outstanding user read all/most of the time.
I also tried to minimize any changes to common code, to try to make sure the feature won't negatively affect any existing scenarios. I still kept most of the the tracing events initially added to the draft implementation with the read-ahead (#105672). The read-ahead capability will be added separately.
Fixes #48729
/cc @stephentoub @BrennanConroy @rzikm @karelz @dotnet/ncl
Logging:
Testing:
(UPD: 2024-08-08) System.Net.WebSockets.Tests.SocketSendReceivePerfTest report