dotnet / extensions

This repository contains a suite of libraries that provide facilities commonly needed when creating production-ready applications.
MIT License
2.63k stars 751 forks source link

Header Propagation Cannot Be Used in `HybridCache` #5568

Open chrisoverzero opened 1 week ago

chrisoverzero commented 1 week ago

Is there an existing issue for this?

Describe the bug

  1. Add a service depending on HttpClient to the DI container via AddHttpClient from "Microsoft.Extensions.Http". ("inner service")
  2. Add a service depending on inner service and HybridCache to the DI container. ("outer service")
  3. Call a method on inner service from outer service as the factory argument of GetOrCreateAsync on HybridCache.
  4. Configure a header to be propagated from the incoming request to HttpClient. (Say, "etag".)

When the HttpClient-depending method is called from the HybridCache, an InvalidOperationException is thrown, saying that the headers collection has not been initialized. When the call is moved out of GetOrCreateAsync, it succeeds.

Expected Behavior

Steps To Reproduce

Exceptions (if any)

An exception of type 'System.InvalidOperationException' occurred in System.Private.CoreLib.dll but was not handled in user code: 'The HeaderPropagationValues.Headers property has not been initialized. Register the header propagation middleware by adding 'app.UseHeaderPropagation()' in the 'Configure(...)' method. Header propagation can only be used within the context of an HTTP request.'

.NET Version

9.0.100-rc.1.24452.12, 9.0.100-rc.2.24474.11

Anything else?

No response

mgravell commented 1 week ago

Firstly: we should probably move this to dotnet/extensions (where HybridCache resides now); not sure I have access to do that, but...

I'll look at the repro tomorrow; one part of me is thinking "but what when the ambient state differs between callers", but: we already allow that foot-shotgun in the regular delegate scenario, so it s perhaps moot to worry about that specifically here. When I see the repro I might have more context; I'm guessing it is the fact that we don't have the ambient http-request precisely because we fork the onward call and await it (as part of the stampede protection).

mgravell commented 1 week ago

Random thought: one option I was discussing the other day was a new flag to disable the stampede protection. If we did that: we would not be context switching - the primary execution flow could involve the callback. Would losing stampede protection be an acceptable trade for ambient http-request access?

chrisoverzero commented 1 week ago

we should probably move this to dotnet/extensions

Oops, sorry, I didn't realize the library had moved.

we already allow that foot-shotgun in the regular delegate scenario

I'm afraid I'll be no help there – I don't know what that means. 😅 Is GetOrCreateAsync not the expected entry point?

Would losing stampede protection be an acceptable trade for ambient http-request access?

For me, for the project in which I discovered this: yes.

mgravell commented 1 week ago

The GetOrCreateAsync is fine and expected.