dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.35k stars 9.99k forks source link

Propagated headers get mixed up without previous async middleware #15384

Closed orthoplex64 closed 4 years ago

orthoplex64 commented 4 years ago

Describe the bug

When using app.UseHeaderPropagation(); as the first middleware in the pipeline and sending many simultaneous requests, I've observed incorrect headers being propagated sometimes. When inserting any async middleware before header propagation (e.g. app.Use(async (context, next) => await next.Invoke());), all propagated headers are correct.

To Reproduce

Steps to reproduce the behavior (ASP.NET Core 3.0 by default):

  1. Clone this tiny demo project: https://github.com/orthoplex64/dotnet-demos
  2. Execute dotnet run --project MangledHeaders in the cloned repository
  3. Navigate to http://localhost:5000 and see all the wrong headers being returned
  4. Uncomment .Use(async (context, next) => await next.Invoke()) in Startup.cs, re-run and see all the correct headers being returned Reproducible on ASP.NET Core 3.0 and 3.1. There is also a package reference to the ASP.NET Core 2.1/2.2 backport of the header propagation middleware, on which it is reproducible as well.

Expected behavior

The correct headers (i.e. the ones included in the current request) should be propagated on requests sent by HTTP Clients to which the header-propagating message handler has been added.

Screenshots

image

Additional context

I don't think it matters here, but the bug reporting template says to include the output of dotnet --info, so here it is:

.NET Core SDK (reflecting any global.json):
 Version:   3.1.100-preview1-014459
 Commit:    ac3b59712d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.1.100-preview1-014459\

Host (useful for support):
  Version: 3.1.0-preview1.19506.1
  Commit:  bbf5542781

.NET Core SDKs installed:
  2.2.100 [C:\Program Files\dotnet\sdk]
  2.2.203 [C:\Program Files\dotnet\sdk]
  3.0.100 [C:\Program Files\dotnet\sdk]
  3.1.100-preview1-014459 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.2.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.2.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.0-preview1.19508.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.2.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.0-preview1.19506.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.0-preview1.19506.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download
benaadams commented 4 years ago

@davidfowl related? https://github.com/aspnet/AspNetCore/issues/13991

davidfowl commented 4 years ago

Yes it’s the same bug

jkotalik commented 4 years ago

@davidfowl ping

alefranz commented 4 years ago

What's the plan for 3.1? is this bug significantly important to include the fix?

alefranz commented 4 years ago

Due to this subtle bug, it is risky to use the HeaderPropagation middleware at the moment. Is there any chance we can include the workaround https://github.com/aspnet/AspNetCore/pull/15435 in a patch release? The kestrel fix https://github.com/aspnet/AspNetCore/pull/14146 is still in progress and I doubt it will be backported to 3.1

If not possible to patch, should we document the bug somewhere (currently there is no documentation for this middleware) with some steps to take when using the middleware to avoid the bug? (e.g.: making sure this is not used as first async middleware in the pipeline)

/cc @rynowak

davidfowl commented 4 years ago

Agreed, we need to patch this.

alefranz commented 4 years ago

@anurse how can this be moved forward? Should I create a PR with #15435 based on the 3.1 release branch?

analogrelay commented 4 years ago

@alefranz best bet is to send a PR to master and open a separate PR later to backport it. We'll have to take the 3.1 backport PR for approval for servicing.

analogrelay commented 4 years ago

Although I'm not entirely clear on the current state since the linked PR was to master but closed.

If there's a change to be made in master, it's best to start there and open a backport PR for approval. If the only change needed is in 3.1, we can go straight there and seek approval. If you're concerned about doing the work and then having it rejected, we can talk about the impact of the issue and risk of the change here and seek pre-approval.

analogrelay commented 4 years ago

Ok, caught up on the thread and I think I know what's up. @alefranz if you want to open a PR with the content for #15435 targetting release/3.1, go for it. We can take a look and submit it for approval.

alefranz commented 4 years ago

Thank you Andrew for looking into this. I've raised a PR against release/3.1 #18300

analogrelay commented 4 years ago

This was fixed in https://github.com/dotnet/aspnetcore/pull/18300 which will be released in 3.1.3.