HeaderPropagation middleware: MessageHandler throws when used outside of a request

[alefranz]

Describe the bug

The MessageHandler relies on the Middleware to initialize HeaderPropagationValues.Headers to detect misconfiguration. However this means the Messagehandler can not be used outside of an http request and currently throws:

System.InvalidOperationException: The HeaderPropagationValues.Headers property has not been initialized. If using this HttpClient as part of an http request, register the header propagation middleware by adding 'app.UseHeaderPropagation() in the 'Configure(...)' method. Otherwise, use HeaderPropagationProcessor.ProcessRequest() before using the HttpClient.

To Reproduce

Steps to reproduce the behavior:

Code example

public void ConfigureServices(IServiceCollection services)
{
    services
        .AddHttpClient("test")
        .AddHeaderPropagation();
    services.AddHostedService<SampleHostedService>();
}

public class SampleHostedService : IHostedService
{
    private readonly IHttpClientFactory _httpClientFactory;
    private readonly HeaderPropagationProcessor _headerPropagationProcessor;
    private readonly ILogger _logger;

    public SampleHostedService(IHttpClientFactory httpClientFactory, HeaderPropagationProcessor headerPropagationProcessor, ILogger<SampleHostedService> logger)
    {
        _httpClientFactory = httpClientFactory ?? throw new ArgumentNullException(nameof(httpClientFactory));
        _headerPropagationProcessor = headerPropagationProcessor ?? throw new ArgumentNullException(nameof(headerPropagationProcessor));
        _logger = logger ?? throw new ArgumentNullException(nameof(logger));
    }

    public Task StartAsync(CancellationToken cancellationToken)
    {
        return DoWorkAsync();
    }

    private async Task DoWorkAsync()
    {
        _logger.LogInformation("Background Service is working.");

        var client = _httpClientFactory.CreateClient("test");
        var result = await client.GetAsync("http://localhost:62013/forwarded");

        _logger.LogInformation("Background Service:\n{result}", result);
    }

    public Task StopAsync(CancellationToken cancellationToken)
    {
        return Task.CompletedTask;
    }
}

Expected behavior

The HttpClient should work if used outside of a request or, if we don't wont to support this use case, the exception should be updated to explain that such use case is not supported..

Possible solutions

I had investigated possible solutions. Not sure what is your preferred options and if we are still in time to do breaking changes on this middleware.
Here are some options:

1. Remove the check that throws the exception which means we would not be able anymore to detect misconfiguration. Can this be replaced with an analyzer?
   On top of the risk that the feature might not work due to misconfiguration, there is also the risk that having the valuefilter not execute (e.g. to set some headers with default values) when not called from outside a request can be unexpected.
2. Update the exception message to also explain that a HttpClient with the HeaderPropagationMessageHandler can not be used outside of a request. This is not ideal as it means you end up duplicating your HttpClients configuration.
3. Extract the functionality from the Middleware to a generic Processor that can be called outside a http request. This will not change the middleware behaviour, but will allow to explicitly call the Processor when consuming an HttpClient outside of a http request. Ideally this will also include changing the valueFactory to receive a headers collection instead of the HttpContext, so that headers can be passed in different scenarios, for example when consuming messages from a queue, however I guess this scenario is out of scope for a middleware. See draft PR HeaderPropagation: add support for hosted services #12170 . The main issue I see with this approach is that when the processor is used directly you must take extra care as you need a different async context per request.

Looking forward for your feedback.

Thank you,
Alessio

/cc @rynowak

[rynowak]

The HttpClient should work if used outside of a request or, if we don't wont to support this use case, the exception should be updated to explain that such use case is not supported..

Can you describe what this would even mean to "support this use case". Is the idea that you want to use header propagation where possible and then degrade gracefully?

I guess I'm more interested to hear about the scenario than the technical changes.

[alefranz]

Hi Ryan,

Is the idea that you want to use header propagation where possible and then degrade gracefully?
that is what I meant. Apologies if my explanation was a bit convoluted.

I believe having some background processing in an API is not so uncommon.
And probably that background process will deal with the same dependent service that some controller's action deal with directly.

When a user add the HeaderPropagation behavior to a client, does not probably realize that this means that the client can not be used anymore outside of a incoming HTTP request transaction (as we rely on the middleware initializing the headers collection to detect misconfiguration).
So there is a requirement of making this constraint more obvious.

Unfortunately when using a typed client and relying on DI, it become convoluted to have to exact same clients just with a different configuration, so I am not sure that this behavior is acceptable.

The alternative that consuming the same client from a hosted service is made possible, but without propagating the headers, to me feels more user friendly, however there could be a scenario when the background processing is triggered by some incoming http request (e.g. storing some data on a queue for later processing by a hosted service in the same process) and a user will incorrectly assume that the headers are propagated and doesn't get any exception to warn about it.
So in this respect the current behavior is more safe, although can be a bit annoying to deal with it.

Does this answer your question or are you looking for an example?

I don't have a real example of a service with such scenario (otherwise I would have caught this before 😅) but it could be a service when you get a request that involve a minimum call to a service A to get the data required to fulfill the response, and it defer to later some more expensive call to service A which is a consequence of the original request but not required to return the response (as it is only a side effect or it will be sent back with a webhook). This could be for example achieved using a a TPL Dataflow ActionBlock or a queuing mechanism than consumed by a hosted service. The key part is that the processor will have to consume the same service A.

I can put together an example if that will help.

Thank you,
Alessio

[rynowak]

Extract the functionality from the Middleware to a generic Processor that can be called outside a http request. This will not change the middleware behaviour, but will allow to explicitly call the Processor when consuming an HttpClient outside of a http request. Ideally this will also include changing the valueFactory to receive a headers collection instead of the HttpContext, so that headers can be passed in different scenarios, for example when consuming messages from a queue, however I guess this scenario is out of scope for a middleware.

I don't really want to do this I guess (anymore so than we already did). The idea of adding this feature to the core was that getting the capture + propagation of values from the current request is a tricky problem that seemed worth ASP.NET Core solving for you.

Writing a general purpose run code to set headers feature is too general, and something that anyone can do with a message handler in whichever way they please.

Put another way: it's worth us providing opinions about how to integrate features of ASP.NET Core with each other, but it's not a great idea for us to provide opinions about all the code you write. It's better to just tell people to use the BCL features than to provide wrappers around them - for us to provide an alternative to some code you could write - there has to be some kind of value to it other than reduction in lines of code

---

I think the scenario makes sense. It's a little surprising that once you opt-in to header forwarding, you can't use that client outside of a request - but it's also a reasonable explanation - but the workaround is painful (duplicate your config)

I think are options are to either:

• Do nothing (can always be fixed in the future)
• Remove the sanity check
• Add an option to bypass the sanity check

We have a few days left to address this before the release really locks down - I've taken a while to respond because I'm not sure which of the last two options is least surprising.

@anurse @Tratcher @davidfowl @glennc

[analogrelay]

We have a few days left to address this before the release really locks down

Barely that even. I think this might be a good candidate to reconsider when we open a branch for 3.1 next week. I'm afraid I haven't been able to catch up on the thread, but is this something that will break customer scenarios if not resolved? If so, we can evaluate it for 3.0 (based on the impact of the bug). If this is just about expanding the usefulness of the HeaderPropagation message handler, I think we'll have to move this out of 3.0. It wouldn't meet the bar.

[rynowak]

There's no break here.

We're discussing how to best deal with a case that would throw an exception in 3.0 - and trying to decide if we want to make it more tolerant. I have no issue with moving this decision to 3.1

[alefranz]

Yes I agree we need to postpone it as we are waiting for more feedback on what is the best approach to take.

@rynowak do we need to rephrase the exception message for 3.0? we could add something at the end to indicate that another possible reason of this exception is that the Http client is used outside of processing of a request which is unsupported.

Thanks!

[rynowak]

Sure, if you want to send a PR that updates the exception message that would be fine. We have until monday to merge it.

[alefranz]

Raised PR #12636