alefranz / HeaderPropagation

ASP.NET Core middleware to propagate HTTP headers from the incoming request to the outgoing HTTP Client requests. This is a backport to ASP.NET Core 2.1 (and 2.2) of the ASP.NET Core HeaderPropagation middleware I had recently contributed to the ASP.NET Core project.
Apache License 2.0
31 stars 1 forks source link

Propagating the last known header value when not present #6

Closed raphaabreu closed 4 years ago

raphaabreu commented 5 years ago

Hello, first of all, awesome project!

I am finding a weird behavior: When I pass a X-Correlation-Id, all works as expected. However, all subsequent outgoing http requests also include the forwarded header with the exact same value even though it was not included in the subsequent incoming requests.

It seems that the last seen value is sticking somehow.

I have set it up with:

public void ConfigureServices(IServiceCollection services) {
...
services.AddHeaderPropagation(options => options.Headers.Add("x-correlation-id"));
services.AddHttpClient<IGeoClient, GeoClient>(client =>
{
    client.BaseAddress = new Uri(configuration.GetConnectionString("GeoGraphQL"));
}).AddHeaderPropagation();
...
}
public void Configure(IApplicationBuilder app) {
...
app.UseHeaderPropagation();
...
}

Am I missing something here?

alefranz commented 5 years ago

Hi Raphael, the configuration is correct. The headers values are store in an AsyncLocal so the value would stick per each async context. If you are using this from a controller action, each call will be a separate async context and the value will be stored correctly. Where are you consuming this from? Are you able to share a bit more code?

raphaabreu commented 5 years ago

I have just noticed that HeaderPropagationValues is registered as a singleton. Shouldn't it be scoped? Otherwise all requests coming in are going to share the same dictionary, right?

https://github.com/alefranz/HeaderPropagation/blob/849f5ec487500ab846171a42ff2c07e00e07e747/src/DependencyInjection/HeaderPropagationServiceCollectionExtensions.cs#L25

I am pretty sure that this would be shared by the entire app, regardless of any request context.

raphaabreu commented 5 years ago

Answering your question, I consume the HttpClient the "standard" way if I can say. The client is registered as a typed client with AddHttpClient and it is retrieved in the service constructor:

public class GeoClient {
...
public GeoClient(HttpClient client, IMapper mapper, ILogger<GeoClient> logger)
...
}

Most actions that access this service come from controllers but I also have it coming from a BackgroundService that creates a dependency injection context for each message being processed from a queue.

I this particular case, there was no messages coming from the queue. This behaviour caught my attention because all internal container healthcheck requests also started to show the exact same header and it should not be possible since this correlation id value was sent only once via Postman.

alefranz commented 5 years ago

Hi Raphael,

I have just noticed that HeaderPropagationValues is registered as a singleton. Shouldn't it be scoped? Otherwise all requests coming in are going to share the same dictionary, right?

it is correct to be a singleton as internally it use an AsyncLocal, see https://github.com/alefranz/HeaderPropagation/blob/7ccc58c16053da032a1d05da77189dfc5af755c8/src/HeaderPropagationValues.cs#L17

Most actions that access this service come from controllers but I also have it coming from a BackgroundService that creates a dependency injection context for each message being processed from a queue.

Currently this library does not support background services. You would have to create two different registration of the HttpClient, one with header propagation to use in the controllers and one without to use in the background services.

I have done some work to support the background services but I haven't released it yet as I was aiming to get the work done in https://github.com/aspnet/AspNetCore and use this library only to backport the feature to 2.x. You can join the discussion on https://github.com/aspnet/AspNetCore/pull/12170 as there is currently not enough interest in this feature to include it in https://github.com/aspnet/AspNetCore.

Would you be interested in using this library if I improve the behavior as per https://github.com/aspnet/AspNetCore/pull/12170 even if that will mean diverging this library from the official HeaderPropagation library part of ASP.NET Core 3.0?

Thank you!

raphaabreu commented 5 years ago

Thank you for the prompt response. To be honest it makes me have mixed feelings because background services evolved so much since 2.0. I would never have thought that I would need two versions of a service just because of header forwarding. I'm definitely joining the discussion on the issue you have pointed out.

As for using the lib with it diverging from the one on 3.0, I think it can cause more harm than good because I myself am looking to upgrading to 3.0 and this would break the behaviour since this lib states that it is a backport.

In any case, at my current project there is no background processing going on at the moment that this behaviour was observed. Is there anything else I can try?

alefranz commented 5 years ago

As long as you are consuming it in a controller action, the headers will be takes from the request. As it is relies on AsyncLocal, make sure you are not doing anything particular with the async context. If you can't recreate a repro that you can share of the behavior you are experiencing it would really help.

alefranz commented 4 years ago

I've released 3.0.2 which address the issue. Please let me know how it goes. Thank you!