Skip to content

Throw on incompatible WebSocket options #74473

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 2, 2022
Merged

Throw on incompatible WebSocket options #74473

merged 4 commits into from
Sep 2, 2022

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Aug 24, 2022 •
edited
Loading

Fixes #74415
Fixes #74416

For #74415, we now throw if HTTP/2 has been requested but a custom invoker wasn't specified.

For #74416, I added a simple check for any options that can't be honored when using a custom invoker instance, and we now throw an ArgumentException.
The only exception is the default Proxy - we don't throw if the user doesn't overwrite it (as that would impact all default use cases). Given that SocketsHttpHandler shares the same default (default proxy is used unless explicitly disabled), I don't think this is a big deal.

@MihaZupan MihaZupan requested review from CarnaViire, greenEkatherine and a team August 24, 2022 02:01
@ghost
Copy link

ghost commented Aug 24, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #74415
Fixes #74416

There were 3 issues around reusing the shared handler:

  1. The bug we had before 7.0 where we weren't reusing the handler unless you manually accessed ClientCertificates
  2. ClientWebSocket sets the Proxy options by default, so we don't reuse the handler unless the user manually sets Proxy back to null
  3. For H2, it's problematic that we set PooledConnectionLifetime to zero, as that means we're opening a new H2 connection for every WebSocket, throwing away all the benefits of the protocol.

I changed the logic so that we now hold 2 shared handler instances: one for "no proxy" and one for "default proxy". This resolves problem nr. 2.

To mitigate problem 3, I changed the default PooledConnectionLifetime from zero to 2 minutes, matching IHttpClientFactory's default. Ideally, we should have a DNS TTL mechanism here instead.
Are we okay with having such a non-configurable default here? This only affects HTTP/2 and has a workaround of using a custom invoker instance.
I also set EnableMultipleHttp2Connections to true on the shared handler to avoid introducing a stream limit (there is no limit if the lifetime is set to zero).

While I was making these changes, I also avoided the HttpMessageInvoker allocation for shared handlers (#70895 (comment)).

For #74416, I added a simple check for any options that can't be honored when using a custom invoker instance, and we now throw an ArgumentException.
The only exception is the default Proxy - we don't throw if the user doesn't overwrite it (as that would impact all default use cases). Given that SocketsHttpHandler shares the same default (default proxy is used unless explicitly disabled), I don't think this is a big deal.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net
Milestone: -

@ghost ghost assigned MihaZupan Aug 24, 2022
@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@CarnaViire
Copy link
Member

It seems that outerloop hit the same issue #74450 did...

CarnaViire
CarnaViire previously approved these changes Aug 24, 2022
View reviewed changes
Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@MihaZupan MihaZupan mentioned this pull request Aug 24, 2022
Closed
@MihaZupan
Copy link
Member Author

@stephentoub could please take a look at this one given that it concerns client sharing/caching?
This is also a place where we could consider using HttpClient.Shared in the future.

@karelz karelz added this to the 8.0.0 milestone Aug 30, 2022
@MihaZupan MihaZupan force-pushed the ws-throw-on-custom-options branch from 0722b7e to dcc51fc Compare September 1, 2022 19:30
@MihaZupan MihaZupan changed the title Reuse shared invoker and throw on incompatible WebSocket options Throw on incompatible WebSocket options Sep 1, 2022
@MihaZupan MihaZupan dismissed stale reviews from greenEkatherine and CarnaViire September 1, 2022 19:32

Stale

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@MihaZupan MihaZupan requested review from stephentoub and a team September 1, 2022 19:33
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

Updated the PR to remove all of the pooling logic. This is now just adding options validation and test changes.

stephentoub
stephentoub approved these changes Sep 1, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product changes LGTM

@MihaZupan MihaZupan closed this Sep 2, 2022
@MihaZupan MihaZupan reopened this Sep 2, 2022
@MihaZupan MihaZupan merged commit 9ea5d2d into dotnet:main Sep 2, 2022
@MihaZupan
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2981306574

@github-actions github-actions bot mentioned this pull request Sep 2, 2022
Merged
@karelz karelz mentioned this pull request Sep 6, 2022
Closed
steveisok pushed a commit to steveisok/runtime that referenced this pull request Sep 9, 2022
Fix WebSockets.Client.ConnectTest crash for apple mobile platforms
48dd367 
dotnet#74473 had a change where we started adding a client certificate that is not supported on apple mobile platforms. This lead to an unhandled exception in the MemberData setup of ConnectAsync_CustomInvokerWithIncompatibleWebSocketOptions_ThrowsArgumentException and resulted in an app crash.

Addresses dotnet#75307
@steveisok steveisok mentioned this pull request Sep 9, 2022
Merged
steveisok added a commit that referenced this pull request Sep 11, 2022
@steveisok
Fix WebSockets.Client.ConnectTest crash for apple mobile platforms (#…
014c36f 
…75344)

#74473 had a change where we started adding a client certificate that is not supported on apple mobile platforms. This lead to an unhandled exception in the MemberData setup of ConnectAsync_CustomInvokerWithIncompatibleWebSocketOptions_ThrowsArgumentException and resulted in an app crash.

Addresses #75307
github-actions bot pushed a commit that referenced this pull request Sep 11, 2022
Fix WebSockets.Client.ConnectTest crash for apple mobile platforms
997473a 
#74473 had a change where we started adding a client certificate that is not supported on apple mobile platforms. This lead to an unhandled exception in the MemberData setup of ConnectAsync_CustomInvokerWithIncompatibleWebSocketOptions_ThrowsArgumentException and resulted in an app crash.

Addresses #75307
steveisok pushed a commit that referenced this pull request Sep 12, 2022
@github-actions
Fix WebSockets.Client.ConnectTest crash for apple mobile platforms (#…
8f18a0d 
…75423)

Backport of #75344 to release/7.0

#74473 had a change where we started adding a client certificate that is not supported on apple mobile platforms. This lead to an unhandled exception in the MemberData setup of ConnectAsync_CustomInvokerWithIncompatibleWebSocketOptions_ThrowsArgumentException and resulted in an app crash.

Addresses #75307
@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@CarnaViire CarnaViire CarnaViire left review comments

+1 more reviewer

@greenEkatherine greenEkatherine greenEkatherine left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@MihaZupan MihaZupan

Labels
area-System.Net
Projects
None yet
Milestone
8.0.0
5 participants
@MihaZupan @CarnaViire @stephentoub @greenEkatherine @karelz