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

[API] Add per request headers override to HttpClient convenience methods #23322

Open
benaadams opened this issue Aug 25, 2017 · 66 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@benaadams
Copy link
Member

Motivation

Custom per request http headers importance are on the rise due to their role in authentication and authentication in newer api security models.

The way to send custom per request headers with HttpClient is via SendAsync and adding them to the HttpRequestMessage.

However that isn't very discoverable and is a big jump from using the convenience methods GetAsync, PostAsync, etc

This can lead people to changing the DefaultRequestHeaders on the HttpClient instead: SO, SO, SO, etc

Which is very dangerous if the HttpClient is used as a static; which is also an official suggested practice

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Especially in server applications, creating a new HttpClient instance for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors.

And in popular blogs "You're using HttpClient wrong and it is destabilizing your software"

This is dramatic. If you have any kind of load at all you need to remember these two things:

  1. Make your HttpClient static.
  2. Do not dispose of or wrap your HttpClient in a using unless you explicitly are looking for a particular behaviour (such as causing your services to fail).

Both changing DefaultRequestHeaders per request and concurrently making requests from a shared HttpClient will end very badly.

Proposal

Add overloads to the convenience methods GetAsync, PostAsync, etc to allow for customer headers on a per request basis from a shared HttpClient

public partial class HttpClient
{
    Task<HttpResponseMessage> DeleteAsync(string requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> DeleteAsync(string requestUri, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> DeleteAsync(Uri requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> DeleteAsync(Uri requestUri, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> GetAsync(string requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> GetAsync(string requestUri, HttpRequestHeaders requestHeaders, HttpCompletionOption completionOption) { throw null; }
    Task<HttpResponseMessage> GetAsync(string requestUri, HttpRequestHeaders requestHeaders, HttpCompletionOption completionOption, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> GetAsync(string requestUri, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> GetAsync(Uri requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> GetAsync(Uri requestUri, HttpRequestHeaders requestHeaders, HttpCompletionOption completionOption) { throw null; }
    Task<HttpResponseMessage> GetAsync(Uri requestUri, HttpRequestHeaders requestHeaders, HttpCompletionOption completionOption, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> GetAsync(Uri requestUri, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<byte[]> GetByteArrayAsync(string requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<byte[]> GetByteArrayAsync(Uri requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<Stream> GetStreamAsync(string requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<Stream> GetStreamAsync(Uri requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<string> GetStringAsync(string requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<string> GetStringAsync(Uri requestUri, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> PostAsync(string requestUri, HttpContent content, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> PostAsync(string requestUri, HttpContent content, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> PostAsync(Uri requestUri, HttpContent content, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> PostAsync(Uri requestUri, HttpContent content, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> PutAsync(string requestUri, HttpContent content, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> PutAsync(string requestUri, HttpContent content, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
    Task<HttpResponseMessage> PutAsync(Uri requestUri, HttpContent content, HttpRequestHeaders requestHeaders) { throw null; }
    Task<HttpResponseMessage> PutAsync(Uri requestUri, HttpContent content, HttpRequestHeaders requestHeaders, CancellationToken cancellationToken) { throw null; }
}

Notes

There are also suggestions for going further for increased safety

I would opt for taking headers at construction time and freeze them, or allow to pass frozen headers through a param or TLS...etc

Perhaps freezing the DefaultRequestHeaders on first request; and throwing an InvalidOperationException if they are modified after the first request has been initiated? As modifying the default headers while requests are being initiated in parallel will lead to nonsensical outcome; and modifying them and submitting a request can't be an atomic operation.

/cc @davidsh, @CIPop, @Priya91 @xoofx @khellang

@darrelmiller
Copy link

I can't say I'm enthusiastic about adding even more overloads to HttpClient. I would be happy with being able to do this:

            httpClient.SendAsync(new HttpRequestMessage()
            {
                RequestUri = new Uri("http://api.example.org"),
                Method = HttpMethod.Post,
                Content = new StringContent("Hello World"),
                Headers = new HttpRequestHeaders() { new AuthenticationHeaderValue("bearer","token")}
            });

Unfortunately you can't because Headers is a read-only property.

@davidfowl
Copy link
Member

Why not a Func instead?

@benaadams
Copy link
Member Author

Alternative, could be extension methods that call through to SendAsync; would be able to ship OOB then also and work at .NET Standard 1.1?

public static class HttpClientExtensions
{
    Task<HttpResponseMessage> GetAsync(
        this HttpClient client, 
        string requestUri, 
        HttpRequestHeaders requestHeaders);
}

@benaadams
Copy link
Member Author

benaadams commented Aug 25, 2017

Why not a Func instead?

Something like?

Task<HttpResponseMessage> GetAsync(
    string requestUri, 
    Action<HttpRequestMessage, object> modifyRequest, 
    object state);

Might not help with discoverablity for new users though?

@davidfowl
Copy link
Member

@benaadams yea or maybe:

Task<HttpResponseMessage> GetAsync(
    string requestUri, 
    Func<HttpRequestMessage, object, Task> modifyRequest, 
    object state);

Might not help with discoverablity for new users though?

True, but less allocatey.

@tmds
Copy link
Member

tmds commented Aug 25, 2017

Custom per request http headers importance are on the rise due to their role in authentication and authentication in newer api security models.

I guess this involves a TokenManager of some sorts. Would be nice to see how that fits into the proposed APIs.

@SteveDesmond-ca
Copy link
Contributor

SteveDesmond-ca commented Aug 25, 2017

I like the idea of passing in a headers object rather than a modifier func, though that could lead to devs doing the same thing and just modifying the same shared header collection.

Can this be solved with documentation? I feel like there's a lot of "you're doing it wrong" out there without a great discussion of trade-offs, e.g.

  • when to use a singleton HttpClient vs not
  • what happens when you do vs don't (good and bad for both)
  • when to use the happy path convenience methods vs creating a message and passing it into the lower-level Send()
  • default headers means default (stop changing them)

OK, now I kind of want to do this...

@darrelmiller
Copy link

darrelmiller commented Aug 25, 2017

I agree with @SteveDesmond-ca that documentation would be a really good solution to this problem. There really isn't much out there on HTTP and there is plenty to cover. I know because I have 6 hours worth of unfinished Pluralsight course material just on HttpClient.

If you take @davidfowl 's Func suggestion to the extreme you end up close to what I have been working with for a long time now which is the notion of a Link object which implements IRequestFactory. It's like the client version of an ActionResult and encapsulates everything about creating a HttpRequestMessage.

So you end up doing this:

var httpClient = new HttpClient();
var link = new Link() { Target= new Uri("http://example.org/foo" };
var response = await httpClient.SendAsync(link.CreateRequest());

and you have the option of going all the way to stuff like this,

var httpClient = new HttpClient();
var link = new UPSTrackingLink() { 
                     TrackingNumber = "Z12323D2323S"
                };
var response = await httpClient.SendAsync(link.CreateRequest());

https://github.com/tavis-software/tavis.link#examples

There are a ton of other benefits to this approach, but I will not attempt to derail further :-)

@darrelmiller
Copy link

@tmds I would use a HTTP Message handler just like on the server. Like this https://github.com/tavis-software/Tavis.Auth/blob/master/src/Auth/AuthMessageHandler.cs

@benaadams
Copy link
Member Author

HttpRequestHeaders doesn't have a public constructor so am trying a different api

@darrelmiller
Copy link

@benaadams It used to, 8 years ago, before it got re-written :-) http://www.bizcoder.com/httpclient-the-basics

@tmds
Copy link
Member

tmds commented Aug 25, 2017

@tmds I would use a HTTP Message handler just like on the server.

@darrelmiller 👍 👍

@adrianm64
Copy link

adrianm64 commented Oct 9, 2017

Sorry to jump in late here and I have probably misunderstood something.

Why is everyone so eager to reuse the HttpClient? I have read the linked articles but they all seem to miss the fact that HttpClient class is more or less just a holder class for headers, timeout and some other things. All methods hitting the network are delegated into the pipeline via HttpMessageHandler.SendAsync.
There is no network state in HttpClient, Dispose does nothing if you pass in your own HttpMessageHandler.

As long as you use the same HttpMessageHandler you can create and dispose HttpClients as much as you want without any penalty (except some memory and a cancellation token). I use code like this:

private static Lazy<HttpClientHandler> _defaultHandler = new Lazy<HttpClientHandler>()

// Allow users to specify their own handler e.g. for logging
public HttpMessageHandler MessageHandler { get; set; }

private HttpClient CreateClient() {
    var client = new HttpClient(MessageHandler ?? _defaultHandler.Value, disposeHandler: false);
    // set base url, headers etc
    return client;
}

public async Task<string> GetStringAsync(Uri uri) {
    using (var client = CreateClient()) {
        return await client.GetStringAsync(uri);
    }
}

Makes it a lot easier to deal with changing headers, timeout etc.

In hindsight it would probably been better to not have the factory-like default constructor and named the class HttpMessageFacade. It would have been slightly more difficult to use but less confusing about what it is.

Edit: forgot an await
Edit2: adaptor -> facade (I should really blow the dust of my GoF book :-))

@robertmiles3
Copy link

FWIW, the extension methods of ServiceStack’s HTTP Utils are a treat. Adding headers is super easy, and something along the lines of @davidfowl suggestion.

http://docs.servicestack.net/http-utils

@yahorsi
Copy link

yahorsi commented Oct 23, 2017

Could we consider adding overload that just takes Bearer token and so adds corresponding header? That would make many today's call's very simple. Or that is too specific?
'

@yahorsi
Copy link

yahorsi commented Oct 23, 2017

@adrianm64 According to sources there is no network sattce in the HttpClientHandler as well. The only thing it does in the Dispose is a call to the ServicePointManager.CloseConnectionGroups(this.connectionGroupName);

So, theoretically, it should be just enough to set disposeHandler: false

PS: Strange, made the test, if HttpClientHandler is newed every time and disposeHandler: false then we see lot's of ports in the CLOSE_WAIT stait. If HttpClientHandler is reused then everything is fine. I twice looked at the HttpClientHandler code and do not understand that. Might be related to the connection group name

@adrianm64
Copy link

@tom3m it is the ServicePointManager.CloseConnectionGroups you want to avoid. You can either do that by never disposing the HttpClient or by using the disposeHandler: false.

@geoffkizer
Copy link
Contributor

You can use collection initializer syntax to do this:

            var t = httpClient.SendAsync(new HttpRequestMessage()
            {
                RequestUri = new Uri("http://microsoft.com"),
                Method = HttpMethod.Get,
                Headers = { { "My-Header", "HeaderValue" } }
            });

It won't work with typed header values since there's no Add method for that, but we could look at adding something like that.

@ericsampson
Copy link

@karelz we ran into this too (per-message custom headers for getasync) because like Ben said originally this is getting to be a more common pattern, is there any interest in reviving this request in some form? Thanks!

@karelz
Copy link
Member

karelz commented Jan 29, 2019

Eventually, we will be interested. Most likely post-3.0.
It has 10 heart-votes, which is on higher end (although I sort only by thumbs up votes - now I have to remember to look at heart-sort as well)

@ericsampson
Copy link

@karelz would this be a good candidate for a community member to submit a PR as a starting point?

@karelz
Copy link
Member

karelz commented Jan 31, 2019

We need API discussion first. It adds lots of methods - is it worth it? That's the important question IMO.

@vladiliescu
Copy link

The current API makes it really really hard to fall into the pit of success. As Ben mentioned, the default instinct is to use the convenience methods (GetAsync, PostAsync, DeleteAsync etc), but then how do you set the headers in Get&Delete? You go and modify the DefaultHeaders of course. But the HttpClient is static because that's what all the guides say. And then all hell ensues, but only sometimes. This is such an easy trap to fall into.

@yahorsi
Copy link

yahorsi commented May 24, 2019

IMHO it should have been designed as stateless API, where all parameters needed are passed via method parameters, and of course it should not be IDisposable. And then you could have statefull API on the top of the statefull, when someone would prefer to save some e.g. headers to be used in all requests and so use concrete instance as that instance holds the state

@adrianm64
Copy link

@yahorsi, you already have that.
HttpMessageHandler is what you call stateless (everything passed via parameters) and HttpClient is stateful (holds default headers etc).

Just use a static HttpMessageHandler and then create a new HttpClient whenever you need to alter default headers/timeout etc.

@yahorsi
Copy link

yahorsi commented May 27, 2019

HttpMessageHandler

@adrianm64 Not sure what you mean. SendAsync is protected abstract in the HttpMessageHandler and receives HttpRequestMessage.

@darrelmiller
Copy link

darrelmiller commented May 27, 2019

@yahorsi @adrianm64 You need to use HttpMessageInvoker if you want to call a HttpMessageHandler directly.

@vladiliescu The convenience methods assume that you are not going to want to set per-request headers. This is not a terrible assumption for a reasonable number of scenarios. Having to fall back on creating a HttpRequestMessage and then use SendAsync if you want more control is, in my opinion, a decent way of peeling back the layer's of the onion.

The original proposal by @benaadams talks about per-request headers for auth, but those should be done via a piece of middleware, not by inserting ad-hoc code before every call to the API.

There are very few headers, in my experience, that are per-request that would not be done more effectively as a a piece of middleware.

@vladiliescu
Copy link

vladiliescu commented May 27, 2019

@darrelmiller Agreed, manually creating an HttpRequestMessage and using it for SendAsync is by no means terrible. I do not think that this is what beginners (i.e. people not familiar with HttpClient) will use though. Beginners will use GetAsync and PostAsync and PutAsync because they're just more convenient. And when they'll need to set per-request headers they'll look at the overloads for Get/Put/Post and find nothing there. Some may look at other methods such as SendAsync (I know I wouldn't, why would I look at other methods when Get/Put/Post do almost exactly what I want :) ) and others may conclude that they need to change the DefaultHeaders.

The latter will encounter dragons down the road, and I think this should be mitigated.

@ericsampson
Copy link

and others may conclude that they need to change the DefaultHeaders.

The latter will encounter dragons down the road, and I think this should be mitigated.

aka 'Pit of Success'. This isn't an imaginary scenario, I've seen this issue happen (and fixed it) at a couple companies. Now maybe it can be argued that the original developers were naive or should have known better (used middleware etc), but the story that @vladiliescu laid out does happen FWIW.

@balivo
Copy link

balivo commented Oct 3, 2019

Any news?

I have a scenario where my users add multiple vendors that will have their APIs consumed by my platform, directly "on behalf" of my user...

The per request header is really useful...

@edokan
Copy link

edokan commented Dec 10, 2020

Maybe you should just make an overload of SendAsync virtual, that should solve almost all of the issues as far as I can see, since all other methods use this internally.

public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)

My question is why the overload without HttpCompletionOption is virtual but not this? I generate a client using NSwagUI, as it is recommended by your docs, which uses 3 parameters overload which I cannot override, and add headers on the fly. Just cannot understand the reasoning behind this decision.

@AndriSig
Copy link

AndriSig commented Jul 8, 2021

It really doesn't feel like needing to change bearer tokens between requests should make the process of sending requests THIS much harder..

It doesn't feel like needing to change the text on a header value between requests should make 99% of the damn client just not work for you?

I get that most dotnet servers will just have predefined clients they are talking to. But surely having a pool of bearer tokens that increases as your customerbase increases isn't that uncommon?

For example for registering API Key's a system can use to interact with a customers Workplace server or similar things where there is one key per customer.

I'd be willing to bet a lot that this design has resulted in a fair number of developers creating completely unsafe code where they fiddle with the defaultheaders between requests and worse..

@jemiller0
Copy link

The .NET Core designers, and in particular the ASP.NET team, never met an over-complicated design pattern that they just had to use. Who would ever need to be able to easily change a request header??? Looking forward to whatever comes next, because HttpClient isn't what it should be. As god awful as WebClient is, it seems better in some cases. I don't know what is wrong with people that they feel the need to make what should be simple, such a hassle.

@FabianToledo
Copy link

So the general recommendation is to have only a single HttpClient instance.

However, the real reason for this is that you only want a single HttpMessageHandler, because this is where all the connection pooling goodness comes from.

In this case, you can have your cake and eat it too, with a single shared instance and being able to continue using DefaultRequestHeaders with something like this:

using HttpClientHandler handler = new HttpClientHandler();

async Task WorkWithApiA()
{
    using var client = new HttpClient(handler, false);
    client.DefaultRequestHeaders.Add("X-Secret-Auth-Token", "foo");

    await client.GetAsync("/");
    // ...
}

async Task WorkWithApiB()
{
    using var client = new HttpClient(handler, false);
    client.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", "foo");

    await client.GetAsync("/");
    // ...
}

This is our recommended solution if you want to use default request headers against multiple APIs, while keeping to a single instance. We believe this will be cleaner than asking users to pass a collection to every convenience API call.

@scalablecory I think this is a great solution. Is there any way to add this to the API documentation?
This answer was very dificult to find and it could be the best solution.
Are there any known issues using this approach?
Thanks!!

@geoffkizer
Copy link
Contributor

Is there any way to add this to the API documentation?
This answer was very dificult to find and it could be the best solution.

I agree, we should do a much better job of explaining this in the docs. Any suggestions on how to do so? Where did you look for this information and what was missing?

@AndriSig
Copy link

So the general recommendation is to have only a single HttpClient instance.
However, the real reason for this is that you only want a single HttpMessageHandler, because this is where all the connection pooling goodness comes from.
In this case, you can have your cake and eat it too, with a single shared instance and being able to continue using DefaultRequestHeaders with something like this:

using HttpClientHandler handler = new HttpClientHandler();

async Task WorkWithApiA()
{
    using var client = new HttpClient(handler, false);
    client.DefaultRequestHeaders.Add("X-Secret-Auth-Token", "foo");

    await client.GetAsync("/");
    // ...
}

async Task WorkWithApiB()
{
    using var client = new HttpClient(handler, false);
    client.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", "foo");

    await client.GetAsync("/");
    // ...
}

This is our recommended solution if you want to use default request headers against multiple APIs, while keeping to a single instance. We believe this will be cleaner than asking users to pass a collection to every convenience API call.

@scalablecory I think this is a great solution. Is there any way to add this to the API documentation?
This answer was very dificult to find and it could be the best solution.
Are there any known issues using this approach?
Thanks!!

Does that work with any of the dependency injection patterns?

@IanKemp
Copy link

IanKemp commented Sep 1, 2021

Does that work with any of the dependency injection patterns?

LOLnope, and of course it doesn't work with anything else that modifies the HTTP handlers - so good luck using Polly with this implementation!

But that's the new standard on the .NET team: instead of listening to developers suggesting valid functionality that would make their lives easier and implementing said functionality, the ivory tower architects declare to the peons clustered below: "This API is perfect as currently designed; if it doesn't work for you, you can use this incredibly unintuitive and contrived workaround that doesn't actually work except in certain very specific cases".

It's laziness at its most extreme, and is symptomatic of a team that's no longer interested in delivering comprehensive APIs and fleshing them out; instead being fixated on "what new and shiny things can we cram into the next major version of .NET that will make us look good on our performance reviews"? It's like that Dilbert comic about bugs and writing a new minivan, except in this case it's new APIs that may or may not be useful.

Much as I loathed the old slow Microsoft, at least it didn't do things half-assed like the new one.

@jemiller0
Copy link

You know it's bad, when you seriously consider using WebClient instead. How such fundamental functionality could be made so difficult to use and non-intuitive is beyond me.

@darrelmiller
Copy link

If I were calling two different APIs that have distinct sets of default request headers I would create a HttpClient instance for each API. The guidance to use a single HttpClient is about not creating one per request, not about creating one for all APIs that you might call. At least that's my opinion. This allows you to use named HttpClient instances to support each API. Chances are you not going to be able to share connections between those APIs anyway as they will likely be on different hosts.

@IanKemp
Copy link

IanKemp commented Sep 1, 2021

If I were calling two different APIs that have distinct sets of default request headers I would create a HttpClient instance for each API. The guidance to use a single HttpClient is about not creating one per request, not about creating one for all APIs that you might call. At least that's my opinion. This allows you to use named HttpClient instances to support each API. Chances are you not going to be able to share connections between those APIs anyway as they will likely be on different hosts.

And when you need to call the same endpoint, at the same URL, with a different Bearer token because you're doing a data import for multiple clients each with a different token? Doesn't work so well then, does it?

Sure, I can write my own extension method to accommodate this but it's such a common case that I shouldn't have to. The method signatures proposed by @benaadams nearly half a decade ago are simple, straightforward, and would solve my problem and the problems of thousands of other devs. Yet the .NET team refuses to implement it, with no adequate explanation for why not.

@karelz
Copy link
Member

karelz commented Sep 1, 2021

@IanKemp if you read the messages above, you can get the picture why we are hesitant -- for example: #23322 (comment), then the fact there are workarounds we believe/believed are sufficient for most cases (see #23322 (comment)).
Overall, adding APIs comes with burden and confusion for developers. So if we add something, we want to make sure it is going to be flexible enough for all needs, while not being overwhelming -- and as you can see in the discussion above, this API does not have a clear consensus on the API shape. Sadly, API design is rarely as simple as one might think.

I would also challenge your claim "it's such a common case":
It took 2 years to get to 10 heart-votes (see #23322 (comment)) and now it is at 28 heart-votes after yet another 2 years. While it is higher in Networking team list, it was at position 5 in Networking and at position ~77 in overall dotnet/runtime repo last year (2020/8). It is now at position 3 in Networking (because we fixed 2 issues from the top5 list during 6.0) and at position 79-86 overall in dotnet/runtime repo.
Why do I know these stats? Because we look at most upvoted issues as one of the input into our planning for each release. And we are aware of this one, yet we are also aware of the fact there are more pressing problems, new protocols to support, code/test debt, etc. in the space.

BTW: I understand you are frustrated, however I would like to encourage you to participate in discussions with calmer tone. Try to understand first why things are the way they are before accusing others. Not everyone would be interested in engaging in a discussion when they feel under attack.
Especially broad statements/accusations don't encourage constructive discussion.

Now, stepping back to the scenario discussed above:

@edokan
Copy link

edokan commented Sep 1, 2021

@karelz Here is my two cents. I support you on the constructive language, but I support @IanKemp on his view. As a long time Microsoft user/developer, I'm really frustrated with almost every decision Microsoft made, but well this is business, you do you. The thing is when Microsoft decides to do something they do it at the expense of the developers and testers, and claim "Developers, developers, developers."

I can rant about why 64 bit VS is useless after too many years advocating for it, or get angry with why there is no clear documentation on ReFS API; I can argue why you should open source VB5 and VB6 (because threading, duh), but I would do that accompanied with a few beers and and a good laugh, as it is supposed to be. I'm frustrated on many subjects, but I do not get angry on them anymore, so should you too, @IanKemp, it is not good for your heart :)

On the other hand it is a well known fact that you cannot make everyone happy, so it does not matter how well you design an API, there always will be a hater. You can argue upvotes are not enough to consider any issue a priority, and it would be a good argument only if Microsoft has not ignored years and years of issues/suggestions in many sites like uservoice/forums and force developers away from itself. I, for one, never upvote/downvote anything MS anymore, knowing it is a futile attempt and fruitless at the end. You will never get real numbers for anything. Many users will just create an HttpClient, download something, and forget even to dispose it. Your real audience are the people who develop nuget libraries that uses every feature of this to overcome many hurdles. Some of these workaround may cause unforeseeable bugs in end user's code. This causes many silent users, but few committed ones.

I do think HttpClient as it is very useful for many common scenarios, but fails to adapt for more complex requirements by not having an intuitive way. Many articles I've read about over the years on many http client implementations by Microsoft all advised different things and provided completely different approaches. Most of the questions about this library would ask if we should create multiple HttpClient objects or not, or if it will exhaust ports/handles or whatnot, or worse yet, it will perform terrible because we forgot to set some flags using intricate calls (GetService, anyone?). It is good thing "The API shape itself is still up to discussion / design.", but your arguments in this discussion are not as strong as you like to believe, and it creates an impression of that it may not be as "up to discussion" as we want it to be.

I am sorry for hijacking the issue and for my rusty English, but just wanted to give everyone another insight. Nonetheless, I'd like to think I've been very civil on this comment.

@benaadams
Copy link
Member Author

benaadams commented Sep 4, 2021

So the general recommendation is to have only a single HttpClient instance.

However, the real reason for this is that you only want a single HttpMessageHandler, because this is where all the connection pooling goodness comes from.

Now set an outbound cookie; where unless I'm mistaken you need an new HttpClientHandler per HttpClient request if the cookies differ?

@CuteLeon
Copy link

CuteLeon commented Oct 10, 2021

Also met this issue when try to add Bearer authentication token before each request. I created a new AuthenticatedHttpClient class and inherit HttpClient just like below:

public class AuthenticatedHttpClient : HttpClient
{
    private readonly IHackSystemAuthenticationTokenHandler authenticationTokenHandler;
    private readonly HackSystemAuthenticationOptions options;

    public AuthenticatedHttpClient(
        IHackSystemAuthenticationTokenHandler authenticationTokenHandler,
        IOptionsSnapshot<HackSystemAuthenticationOptions> optionsSnapshot)
    {
        this.authenticationTokenHandler = authenticationTokenHandler;
        this.options = optionsSnapshot.Value;
        this.BaseAddress = new Uri(this.options.AuthenticationURL);
    }

    protected async Task AddAuthorizationHeader()
    {
        var token = await this.authenticationTokenHandler.GetTokenAsync();
        this.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(this.options.AuthenticationScheme, token);
    }


    public override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        this.AddAuthorizationHeader().ConfigureAwait(false);
        return base.Send(request, cancellationToken);
    }

    public override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        await this.AddAuthorizationHeader();
        return await base.SendAsync(request, cancellationToken);
    }
}

I override Send and SendAsync methods to add currently token into header before each request, and this solution works well for POST requests, but doesn't work when I call GetFromJsonAsync<> extension method.
I found this page which opened 4 years ago when going to find a solution on internet.

@CuteLeon
Copy link

CuteLeon commented Oct 11, 2021

A new solution, works well so far.
CuteLeon/HackSystem@2d31b63

public static IServiceCollection AddHackSystemAuthentication(
    this IServiceCollection services,
    Action<HackSystemAuthenticationOptions> configure)
{
    services
        .Configure(configure)
        .AddTransient<AuthenticatedDelegatingHandler>()
        .AddHttpClient(AuthenticatedServiceBase.AuthenticatedClientName)
        .ConfigureHttpClient((sp, client) =>
        {
            var options = sp.GetRequiredService<IOptions<HackSystemAuthenticationOptions>>().Value;
            client.BaseAddress = new Uri(options.AuthenticationURL);
        })
        .AddHttpMessageHandler<AuthenticatedDelegatingHandler>();

    return services;
}
public class AuthenticatedDelegatingHandler : DelegatingHandler
{
    private readonly IHackSystemAuthenticationTokenHandler authenticationTokenHandler;
    private readonly HackSystemAuthenticationOptions options;

    public AuthenticatedDelegatingHandler(
        IHackSystemAuthenticationTokenHandler authenticationTokenHandler,
        IOptionsSnapshot<HackSystemAuthenticationOptions> optionsSnapshot)
    {
        this.authenticationTokenHandler = authenticationTokenHandler;
        this.options = optionsSnapshot.Value;
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        await this.AttachAuthorizationHeader(request);
        return await base.SendAsync(request, cancellationToken);
    }

    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        this.AttachAuthorizationHeader(request).ConfigureAwait(false);
        return base.Send(request, cancellationToken);
    }

    protected async Task AttachAuthorizationHeader(HttpRequestMessage request)
    {
        if (request.Headers.Authorization is null)
        {
            var token = await this.authenticationTokenHandler.GetTokenAsync();
            request.Headers.Authorization = new AuthenticationHeaderValue(this.options.AuthenticationScheme, token);
        }
    }
}
public class AuthenticatedServiceBase
{
    public const string AuthenticatedClientName = "AuthenticatedClient";
    private readonly IHttpClientFactory httpClientFactory;
    protected readonly ILogger<AuthenticatedServiceBase> logger;

    protected HttpClient HttpClient { get; init; }

    public AuthenticatedServiceBase(
        ILogger<AuthenticatedServiceBase> logger,
        IHttpClientFactory httpClientFactory)
    {
        this.logger = logger;
        this.httpClientFactory = httpClientFactory;
        this.HttpClient = this.httpClientFactory.CreateClient(AuthenticatedClientName);
    }
}

@zachcwillson
Copy link

Has there been any movement here? I remember seeing somewhere that with net8 there are performance improvements to the HttpClientJsonExtensions.GetFromJsonAsync method. Unfortunately, my sleuthing skills on a Friday afternoon are lacking and I can't find the blog post to link.

I was looking at moving us from HttpClient.SendAsync(HttpRequestMessage) to this but being unable to pass bearer tokens in the header is a blocker.

@ghost
Copy link

ghost commented Sep 14, 2023

Would love to see this get addressed somehow.

For years past I have just leveraged HttpRequestMessage with SendAsync to make various API calls as needed. Occasionally, I would need do something fancy like retry some number of times, manually follow redirects with some specific action etc but it was seldom enough that I would just handled it on a case by case basis.

Unfortunately, micro-service architecture has taken over so much now and the prevalence of per user auth and signature headers has created a very demanding requirement for resilient API requests that support advanced patterns beyond a simple retry or timeout like circuit breaker, bulkhead etc. This has forced many developers to rely on libraries such as Polly to expertly handle these scenarios to achieve the resilient API needed without having to reinvent the wheel again and again.

I as understand it, Polly use GetAsync like convenience methods because the HttpRequestMessage is not reusable and they would have to constantly clone the critical elements of the HttpRequestMessage to new instances to handle many cases which can be problematic.

I am just a common lowly dev trying to get tickets cranked out at work so I can enjoy my nights as weekends.

I appreciate a simple/stable/elegant API contract but I would rather get a functional API that met significant need over simple to use, at least most days of the week. Additionally this issue is only growing and though I have searched solutions to this vary issue many times over the last few years I almost always ended up following the newest SO post with some cautions and caveats before I ever crawled across this thread.

Apologies for running a tangent just to say that per request headers on an HttpClient being built in is still very much needed, regardless of the specific contract complexities it take to make it happen. I hope any agreement to move forward comes soon to address the common developers out there that really need this functionality.

@ericsampson
Copy link

This would probably be a great community PR! If anyone is interested in giving it a shot :)

@yahorsi
Copy link

yahorsi commented Dec 1, 2023

@davidfowl probably didn’t see this long waiting and imho very important suggestion)

@ericsampson
Copy link

@karelz is there any way we can kick off some sort of API design session/proposals?

If that's the appropriate next step, in order to get some proposals/feedback and then consensus on a design that someone can implement.

@davidfowl
Copy link
Member

My 2c

The big reason this is stalled (from my POV) is because nobody on the team is enthused about adding this many overloads to HttpClient. Not only do we need these overloads, but we need overloads for the JSON methods, and any method (extension or not) we add in the future. The design proposed doesn't scale.

Delegating handlers are too much ceremony to add a header, and reverting back to HttpRequestMessage also feels like ceremony. We feel like this is important but nobody has come up with a design that doesn't have this complexity cliff.

@daveylad
Copy link

daveylad commented Dec 7, 2023

Landed on this thread and discovered I appear to be jumping though the same hoops as others on here.. and mainly hitting a wall on the other side. How thoroughly demoralising to see the this conversation started over 6 years ago.. and still going

@ericsampson
Copy link

@davidfowl is there potential in an approach like adding a RequestHeadersFactory Func ?
In effect a "prebuilt targeted/convenience HttpDelegatingHandler"

@Fed03
Copy link

Fed03 commented Dec 15, 2023

@davidfowl what do you think about a method that returns a new HttpClient copy that overrides just what passed inside, leaving the original client untouched?

// somewhere you obtained an HttpClient instance already configured with baseAddress and other stuff

var newClient = _client.WithOptions(client => {
    client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", "123");
});

this way no one would need to create many specific overrides or do fancy delegatingHandlers stuff. Though I don't now if this is possible

@LSuds
Copy link

LSuds commented May 7, 2024

Are conversations still being had on this topic? I think this really deserves more attention - I'm sure there have been many sketchy fixes to account for this. I'm about to make one myself.

@scottmcm
Copy link

Which is very dangerous if the HttpClient is used as a static; which is also an official suggested practice

I think perhaps the answer here is just to change that suggested practice in the documentation.

#23322 (comment) from almost 7 years ago is a great way -- just share your handler, and use HttpClient.DefaultRequestHeaders to your heart's content by making as many HttpClients as you need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests