HeaderPropagation: reset AsyncLocal per request

[alefranz]

As Kestrel can bleed the AsyncLocal across requests, see #13991.

Backported from #15435, this change will not be needed on master, as the underlying Kestrel issue will be probably fixed with #14146.

Addresses #15384

/cc @anurse

[jkotalik]

@anurse assigning you to champion for shiproom/update the PR.

From what I can tell, the PR looks good.

[analogrelay]

From what I can tell, the PR looks good.

It's easier to get approvals if we have review signoffs from folks, so @Tratcher @davidfowl @halter73 please also take a look.

[davidfowl]

Can we make this new up one every time as well?

[alefranz]

We could but it would be a breaking change. Any particular reason?

[Tratcher]

I'm not sure why there's even a null check here? Why isn't this always initialized here? Who else would be initializing Headers before this component? If you had multiple instances of the middleware?

[davidfowl]

Right. We should just change it to create a new one every time. I'm not sure why this is being stored in the first place (that's what caused the original bug).

[Tratcher]

HeaderPropagationValues is the async local that's consumed later, so it does need to be set. I'm just not sure why it bothers to null check before setting it.

[halter73]

Are you asking for this to be changed to the following @davidfowl?

var headers = _values.Headers = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase);

So _values.Headers is always set to an empty dictionary at the start of the middleware? @alefranz is correct that this is breaking. Here's an example of the code this would break. Not sure how realistic/common this is, but we probably want to be careful in a patch.

public void ConfigureServices(IServiceCollection services)
{
    services.AddHeaderPropagation(options =>
    {
        options.Headers.Add("User-Agent");
    });
})

// ...

public void Configure(IApplicationBuilder app, HeaderPropagationValues headerPropagationValues)
{
    app.Use((_, next) =>
    {
        // Always "propagate" a "Foo: Bar" header.
        headerPropagationValues.Headers = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase)
        {
            { "Foo", "Bar" }
        };

        return next();
    });

    app.UseHeaderPropagation();

    app.Run(async context =>
    {
        context.Response.Headers["Content-Type"] = "text/plain";

        foreach (var entry in headerPropagationValues.Headers)
        {
            await context.Response.WriteAsync($"{entry.Key}: {entry.Value}\r\n");
        }
    });
}

Today, this would print "Foo: Bar" to the response in addition to the propagated User-Agent. After newing up the dictionary every time, it would only include the propogated User-Agent.

Unless you mean the following:

var headers = _values.Headers = _values.Headers != null ?
    new Dictionary<string, StringValues>(_values.Headers, StringComparer.OrdinalIgnoreCase) :
    new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase);

That makes some sense. It would allow my sample to use a static "Foo: Bar" dictionary instead of newing one up each time since it would have to worry about HeaderPropagationMiddleware mutating it. Technically if someone was relying on the mutation, that could be breaking too, but I think that's less likely to be an issue.

[halter73]

@davidfowl I'm assuming you consider #14146 as too risky for a patch. Would you consider taking a patch with something like #14027 instead? That would also fix the AsyncLocal issue for people who aren't using HeaderPropagation.

[analogrelay]

I'm assuming you consider #14146 as too risky for a patch.

I'd consider it risky for a patch too :). It's also a much more impactful change whereas this focuses on the area in which we have a direct customer report of a problem.

[analogrelay]

@davidfowl @halter73 can we land the question of which kind of change we want for 3.1 so I can take something clear to shiproom? ;)

Removing servicing-consider for now.

[halter73]

I'm fine with the change as it is now. It is an incremental improvement.

[davidfowl]

LGTM

[jkotalik]

@anurse make sure you get this into 3.1.3 😄

[analogrelay]

@anurse make sure you get this into 3.1.3 😄

Yep, branches aren't open but it's on my list :)