Skip to content
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

iOS/tvOS/MacCatalyst: some HttpClientHandler properties behave differently now #55986

Closed
rolfbjarne opened this issue Jul 20, 2021 · 8 comments · Fixed by #57555
Closed

iOS/tvOS/MacCatalyst: some HttpClientHandler properties behave differently now #55986

rolfbjarne opened this issue Jul 20, 2021 · 8 comments · Fixed by #57555
Assignees
Milestone

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Jul 20, 2021

Description

I'm not sure if this is a bug or expected behavior now, but I'm filing this issue to track whatever it is.

This happens on iOS, tvOS or Mac Catalyst (not macOS).

  1. The three Supports* properties all return false now, they used to return true. The corresponding supported properties throw PlatformNotSupportedException (which I guess is understandable if they're not supported).
  2. Setting ServerCertificateValidationCallback throws a PNSE. This used to work.
  3. CookieContainer returns null.

Regression?

Yes, it happened sometime between 6.0.100-preview.7.21330.1 and 6.0.100-rc.1.21369.3 (it's also a breaking change from legacy Xamarin).

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jul 20, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@rolfbjarne
Copy link
Member Author

CC @steveisok @spouliot

@steveisok steveisok self-assigned this Jul 20, 2021
@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2021
@steveisok
Copy link
Member

@rolfbjarne What we are doing with HttpClientHandler in iOS is mapping the properties that are implemented in NSUrlSessionHandler and the ones that aren't will throw PNSE. The reason why this is different in P7 is that the properties reflected what was supported in SocketsHttpHandler.

  1. The three Supports* properties all return false now, they used to return true. The corresponding supported properties throw PlatformNotSupportedException (which I guess is understandable if they're not supported).

I don't think any of these features the three properties reference are supported by NSUrlSessionHandler. If that's not the case, please let me know.

  1. Setting ServerCertificateValidationCallback throws a PNSE. This used to work.

Was the callback actually invoked from the handler?

  1. CookieContainer returns null.

We're deferring to the native handler for any defaults. You'll need to initialize them in your handler.

@rolfbjarne
Copy link
Member Author

OK, I think I was confused, and hopefully I've figured things out now.

In legacy Xamarin, we can select at build time which handler is the default for HttpClient (be it NSUrlSessionHandler, HttpClientHandler or CFNetworkHandler). So the build time option affects how "new HttpClient ()" behaves.

In .NET, we can select at build time which implementation HttpClientHandler should use (be it SocketsHttpHandler or NSUrlSessionHandler). So the build time option affects how "new HttpClientHandler ()" behaves (and thus indirectly how "new HttpClient ()" behaves as well).

In our tests we were doing "new HttpClientHandler ()", and assuming that that would get us a specific implementation. In .NET that's not true anymore, it depends on a build-time option.

The end result is that we'll have to update our tests, and afaik there's nothing to do here.

@rolfbjarne
Copy link
Member Author

@steveisok

Reopening again, because I'm not sure I understand the rationale here:

@rolfbjarne What we are doing with HttpClientHandler in iOS is mapping the properties that are implemented in NSUrlSessionHandler and the ones that aren't will throw PNSE. The reason why this is different in P7 is that the properties reflected what was supported in SocketsHttpHandler.

This means that if you disable the native handler (set UseNativeHttpHandler=false), you'll use SocketsHttpHandler, except for the properties that aren't supported in NSUrlSessionHandler, which will throw PNSE.

Shouldn't those properties instead do whatever the underlying handler does, instead of assuming that if they aren't supported in NSUrlSessionHandler, they won't be supported in SocketsHttpHandler either?

@rolfbjarne rolfbjarne reopened this Jul 23, 2021
@steveisok
Copy link
Member

Good point. We thought to do PNSE mostly because NSUrlSessionHandler does not have most of the properties that you find in HttpClientHandler. If we stop throwing PNSE in runtime , what that means is NSUrlSessionHandler will need to add all of the HttpClientHandler properties. Otherwise, you'll get a rather uninformative reflection exception.

@steveisok steveisok added this to the 6.0.0 milestone Jul 26, 2021
@campersau
Copy link
Contributor

The same has been done to the browser http handler last year: #39760

@rolfbjarne rolfbjarne changed the title iOS/tvOS/MacCactalyst: some HttpClientHandler properties behave differently now iOS/tvOS/MacCatalyst: some HttpClientHandler properties behave differently now Aug 3, 2021
@rolfbjarne
Copy link
Member Author

If we stop throwing PNSE in runtime , what that means is NSUrlSessionHandler will need to add all of the HttpClientHandler properties.

That shouldn't be a problem.

steveisok pushed a commit to steveisok/runtime that referenced this issue Aug 17, 2021
Since HttpClientHandler on mobile can support both the underlying native handler and SocketsHttpHandler, it doesn't make sense to always throw PNSE on some methods due to only the native handler not supporting them.

Fixes dotnet#55986
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 17, 2021
akoeplinger pushed a commit that referenced this issue Aug 17, 2021
Since HttpClientHandler on mobile can support both the underlying native handler and SocketsHttpHandler, it doesn't make sense to always throw PNSE on some methods due to only the native handler not supporting them.

Fixes #55986

Co-authored-by: Steve Pfister <steve.pfister@microsoft.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants