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

[browser][http] Fix HttpClientHandler "Supports*" properties regressions #39889

Merged
merged 4 commits into from
Jul 28, 2020
Merged

[browser][http] Fix HttpClientHandler "Supports*" properties regressions #39889

merged 4 commits into from
Jul 28, 2020

Conversation

kjpou1
Copy link
Contributor

@kjpou1 kjpou1 commented Jul 24, 2020

Also needs implementing on SocketsHttpHandler which will default to true.

        internal bool SupportsAutomaticDecompression => true;
        internal bool SupportsProxy => true;
        internal bool SupportsRedirectConfiguration => true;

Fix #39760

@ghost
Copy link

ghost commented Jul 24, 2020

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

On the SocketsHttpHandler we will default to `true` here.

```
        internal bool SupportsAutomaticDecompression => true;
        internal bool SupportsProxy => true;
        internal bool SupportsRedirectConfiguration => true;
```
@kjpou1
Copy link
Contributor Author

kjpou1 commented Jul 28, 2020

@marek-safar | @lewing | @stephentoub could someone take a look

@marek-safar
Copy link
Contributor

Looks good to me, can anyone from @dotnet/ncl review the change ?

Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

Change looks fine.

For the future, our code for automatic decompression and redirect handling is handler-agnostic and might easily be integrated into BrowserHttpHandler. See how SocketsHttpHandler sets them up here by layering handlers:

private HttpMessageHandlerStage SetupHandlerChain()
{
// Clone the settings to get a relatively consistent view that won't change after this point.
// (This isn't entirely complete, as some of the collections it contains aren't currently deeply cloned.)
HttpConnectionSettings settings = _settings.CloneAndNormalize();
HttpConnectionPoolManager poolManager = new HttpConnectionPoolManager(settings);
HttpMessageHandlerStage handler;
if (settings._credentials == null)
{
handler = new HttpConnectionHandler(poolManager);
}
else
{
handler = new HttpAuthenticatedConnectionHandler(poolManager);
}
if (settings._allowAutoRedirect)
{
// Just as with WinHttpHandler, for security reasons, we do not support authentication on redirects
// if the credential is anything other than a CredentialCache.
// We allow credentials in a CredentialCache since they are specifically tied to URIs.
HttpMessageHandlerStage redirectHandler =
(settings._credentials == null || settings._credentials is CredentialCache) ?
handler :
new HttpConnectionHandler(poolManager); // will not authenticate
handler = new RedirectHandler(settings._maxAutomaticRedirections, handler, redirectHandler);
}
if (settings._automaticDecompression != DecompressionMethods.None)
{
handler = new DecompressionHandler(settings._automaticDecompression, handler);
}
// Ensure a single handler is used for all requests.
if (Interlocked.CompareExchange(ref _handler, handler, null) != null)
{
handler.Dispose();
}
return _handler;
}

@kjpou1 kjpou1 merged commit 1d05f27 into dotnet:master Jul 28, 2020
@kjpou1
Copy link
Contributor Author

kjpou1 commented Jul 28, 2020

@scalablecory Those two things are taken care of via the browser's fetch implementation such that we do not have control of the functionality. Really outside of the domain we can touch or control.

@kjpou1 kjpou1 deleted the wasm-issue-39760-properties-regression branch July 29, 2020 04:36
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
…ons (dotnet#39889)

* [browser][http] Fix HttpClientHandler "Supports*" properties regressions

* Also needs implementing on SocketsHttpHandler.

On the SocketsHttpHandler we will default to `true` here.

```
        internal bool SupportsAutomaticDecompression => true;
        internal bool SupportsProxy => true;
        internal bool SupportsRedirectConfiguration => true;
```

* Should be internal accessor
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net.Http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blazor WebAssembly HttpClientHandler "Supports*" properties regressions in 5.0.0-preview.7.20365.19
5 participants