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

Propagated headers get mixed up without previous async middleware #15384

Closed
orthoplex64 opened this issue Oct 24, 2019 · 12 comments
Closed

Propagated headers get mixed up without previous async middleware #15384

orthoplex64 opened this issue Oct 24, 2019 · 12 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Milestone

Comments

@orthoplex64
Copy link

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
Copy link
Member

@davidfowl related? #13991

@davidfowl
Copy link
Member

Yes it’s the same bug

@jkotalik
Copy link
Contributor

@davidfowl ping

@alefranz
Copy link
Contributor

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

@alefranz
Copy link
Contributor

alefranz commented Jan 7, 2020

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 #15435 in a patch release?
The kestrel fix #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
Copy link
Member

Agreed, we need to patch this.

@alefranz
Copy link
Contributor

alefranz commented Jan 9, 2020

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

@analogrelay analogrelay removed this from the 5.0.0 milestone Jan 9, 2020
@analogrelay
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@analogrelay analogrelay added this to the 3.1.x milestone Jan 15, 2020
@analogrelay
Copy link
Contributor

This was fixed in #18300 which will be released in 3.1.3.

@analogrelay analogrelay modified the milestones: 3.1.x, 3.1.3 Feb 14, 2020
@analogrelay analogrelay added the ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! label Feb 14, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Projects
None yet
Development

No branches or pull requests

9 participants