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

Inline second statemachine into loop and us EC.Restore to reset context #23175

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

benaadams
Copy link
Member

Use EC.Restore to return to clean context rather than introducing a per request statemachine.

This allows use to use the connection request loop statemachine which is pre-allocated; and avoid one of the extra statemachines.

Requires dotnet/runtime#38011 "API Proposal: ExecutionContext.Restore(ExecutionContext? executionContext)"

/cc @stephentoub @halter73

{
await FireOnCompleted();
// Clear any AsyncLocals set during the request; back to a clean state ready for next request
ExecutionContext.Restore(cleanContext);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if an exception occurs prior to this? Is the thread polluted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Either its caught further up; changing the response to a 4xx, 500 etc
image

Or it escapes this method (when the EC.Restore wouldn't run); when the whole connection is burnt and socket is shutdown.

As its running on the ThreadPool the "polluted" thread gets cleaned up when it's returned.

@davidfowl
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@davidfowl
Copy link
Member

@aspnet-hello build

@benaadams
Copy link
Member Author

Will fail the api is only internal currently as its not been approved/reviewed dotnet/runtime#38011

@halter73 halter73 marked this pull request as draft July 30, 2020 22:52
@halter73 halter73 removed their assignment Jul 30, 2020
@halter73 halter73 added the blocked The work on this issue is blocked due to some dependency label Jul 30, 2020
@benaadams
Copy link
Member Author

Change has been merged dotnet/runtime#40322; but don't know how fast it flows through?

@benaadams benaadams closed this Aug 12, 2020
@benaadams benaadams reopened this Aug 12, 2020
@benaadams
Copy link
Member Author

Not yet

@benaadams benaadams marked this pull request as ready for review August 15, 2020 06:07
@benaadams
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 23175 in repo dotnet/aspnetcore

@benaadams
Copy link
Member Author

Not yet

@davidfowl
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@benaadams
Copy link
Member Author

Api was merged to runtime on 7 Jul, any idea how long its taking to flow through these days?

@benaadams benaadams closed this Aug 16, 2020
@benaadams benaadams reopened this Aug 16, 2020
@benaadams
Copy link
Member Author

🎉🥳

@davidfowl davidfowl merged commit 03b75e3 into dotnet:master Aug 17, 2020
@davidfowl
Copy link
Member

Thanks @benaadams

@benaadams benaadams deleted the EC.Restore branch August 17, 2020 01:16
@dougbu dougbu removed the blocked The work on this issue is blocked due to some dependency label Oct 5, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants