[release/7.0] Throw on incompatible WebSocket options #75014
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackport of #74473 to release/7.0 /cc @MihaZupan Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
|
@dotnet/ncl need a code review sign off. |
karelz
added
the
breaking-change
ghost
added
the
needs-breaking-change-doc-created
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
|
Approved -- fixes to likely pain points (2 pits of failure) in new scenario. Few existing users to break, and codepath does not affect existing 1.1 scenario |
|
Breaking change doc issue: dotnet/docs#31430 |
CarnaViire
removed
the
needs-breaking-change-doc-created
ghost
locked as resolved and limited conversation to collaborators
Backport of #74473 to release/7.0
Fixes #74415
Fixes #74416
Customer Impact
We added support for WebSockets over HTTP/2 in .NET 7 RC1. As part of that change, we also introduced a new overload of
ConnectAsyncthat accepts anHttpMessageInvokerto allow reusing existing pooled connections.This PR fixes two issues related to user input validation:
The first is #74416 where existing options available on
ClientWebSocketOptions(e.g.Proxy,RemoteCertificateValidationCallback) would be silently ignored if a customHttpMessageInvokerwas also specified.This PR makes it so that if any incompatible options are set and an invoker is specified, we will throw. This gives the developer a clear signal of what must be changed.
The second issue is #74415, where a developer attempting to utilize the new feature may fall into a performance trap. THE point of WebSockets over H/2 is the ability to multiplex WebSocket streams over a single transport connection. If the user only changes the
VersionorVersionPolicyto allow for H/2, the implementation will use the requested protocol, but it will always open a new connection for each WebSocket, eliminating the benefit of said feature. To get the full benefit of H/2, the user must specify the invoker instance that should be reused by passing it toConnectAsync.This PR makes it so that we throw if H/2 is requested but an invoker was not specified.
Testing
Added a number of targeted unit tests.
Risk
Low - these are new APIs introduced in recent 7.0 RC1 (technically in Preview 7, but the feature had a bug so it did not work), we believe very few users are consuming them already.
For those that are, we believe that scenarios that can be affected are misconfigurations.
With #74416 the options were being ignored, so any testing done by the user relying on these options would show that the scenario is broken. This change just makes debugging the issue easier for the developer.
With #74415 we are talking about a brand new feature that has limited support in the ecosystem (Kestrel is also adding it in 7.0). We believe the number of customers that are already using it with 7.0 RC1 in production is close to none, but we still consider it a breaking change against RC1 and will document it as such.
Any impacted user upgrading from RC1 to 7.0 RC2/RTW that did any testing prior to deployment to production would immediately observe the exception.