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

Use AsyncLocal hold HttpRequestMessage. #65

Merged
merged 5 commits into from
Nov 18, 2021
Merged

Conversation

pengweiqhca
Copy link
Contributor

@pengweiqhca pengweiqhca commented Nov 16, 2021

Fix #64.

@tillig
Copy link
Member

tillig commented Nov 16, 2021

I don't really want this to be a thing where we maintain two different configurable paths for this. If it's going to be AsyncLocal, it just needs to be AsyncLocal. I think that should be available in all clients since we support .NET 4.7.2 minimum here now. Can you remove the switch and just default it to AsyncLocal?

@pengweiqhca
Copy link
Contributor Author

Add switch, no need to consider compatibility issues and will not change the existing behavior.

@tillig
Copy link
Member

tillig commented Nov 17, 2021

I understand the idea, but you're not the one who's going to have to own, maintain, and support it long term. Making it easy to support is kind of important since there aren't actually that many people dedicated here.

If we're going AsyncLocal, let's do it and we can release a minor version to indicate it's a new feature rather than a backwards compatible bug fix.

If we get complaints, we can roll it back or fix forward then.

@pengweiqhca
Copy link
Contributor Author

Switch removed.

@tillig
Copy link
Member

tillig commented Nov 17, 2021

Looking at the way ASP.NET Core uses AsyncLocal in HttpContextAccessor it appears they use some object indirection - a holder for the context instead of storing the context itself - to ensure changing and updating happens across all execution contexts.

Should we do something like that here?

@pengweiqhca
Copy link
Contributor Author

HttpContextAccessor look like a new feature, but I just want to fix the "bug" at the moment.

@tillig
Copy link
Member

tillig commented Nov 18, 2021

I'm not asking you to use HttpContrxtAccessor, I'm suggesting that you may need that "holder" object sort of thing here - the holder contains the message, the AsyncLocal contains the holder.

@pengweiqhca
Copy link
Contributor Author

Modify static HttpRequestMessageProvider to interface IHttpRequestMessageAccessor?

@tillig
Copy link
Member

tillig commented Nov 18, 2021

No. Go look at the code for HttpContextAccessor that I linked. Pretend the HttpRequestMessage here is the HttpContext there. Look at what their AsyncLocal stores. You'll see it does not directly store HttpContext. It stores a holder object. The holder object stores HttpContext. I'm saying you need the holder object in here, and the holder should store the request message.

If this still doesn't make sense you'll have to wait a while for one of us to do the change. I'm heading into some desperately needed time off so basically all I have is my phone. I won't be able to take care of it for a while

@pengweiqhca pengweiqhca changed the title Add AppContext switch, allow use AsyncLocal hold HttpRequestMessage. Use AsyncLocal hold HttpRequestMessage. Nov 18, 2021
Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@tillig tillig merged commit f727953 into autofac:develop Nov 18, 2021
@tillig
Copy link
Member

tillig commented Nov 18, 2021

I'll cut a release as soon as I can. It should end up being 6.1.0.

@tillig
Copy link
Member

tillig commented Nov 18, 2021

Published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change CallContext to AsyncLocal
2 participants