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

Cleanup and document async.fs #11806

Merged
merged 10 commits into from
Jul 9, 2021
Merged

Cleanup and document async.fs #11806

merged 10 commits into from
Jul 9, 2021

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Jul 8, 2021

The process of fixing bug #11797 via #11799 made me go back and review async.fs end-to-end.

This does the following:

  1. Very carefully documenting exactly when cancellation and trampoline/hijack checks are currently made

  2. Fix Should async.TryFinally register the finally block before checking for cancellation? #8675, where a cancellation check was happening before a try/finally was being entered, causing resources to be potentially dropped on the floor if cancellation happens

  3. Cleanup the code that does CancellationTokenRegistration (AwaitWaitHandle, AwaitBeginEnd etc.) to be more uniform

@Liminiens
Copy link

Maybe #8668 could also be addressed? 😇 If it still reproduces on main branch.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 8, 2021

Maybe #8668 could also be addressed? 😇 If it still reproduces on main branch.

I was surprised but as I mentioned in #8668 reducing the allocations by the technique I tried didn't actually improve performance. It might be I wasn't measuring correctly.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 8, 2021

This is ready. Best to review the changes with ignore-whitespace-changes on.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks good.

@dsyme dsyme merged commit 8cce324 into dotnet:main Jul 9, 2021
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.

Should async.TryFinally register the finally block before checking for cancellation?
3 participants