Skip to content

Fix TimerAwaitable rooting #20937

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

Merged
merged 2 commits into from
Apr 18, 2020
Merged

Fix TimerAwaitable rooting #20937

merged 2 commits into from
Apr 18, 2020

Conversation

davidfowl
Copy link
Member

  • This fixes an issue where components that use timer awaitable currently need to be explicitly stopped and disposed for it to be unrooted. This makes it possible to write a finalizer.

This pattern exists in a couple more places in the BCL:

It usually affects client APIs where disposal of resources happen if something is unreferenced.

- This fixes an issue where components that use timer awaitable currently need to be explicitly stopped and disposed for it to be unrooted. This makes it possible to write a finalizer.
@ghost ghost added the area-signalr Includes: SignalR clients and servers label Apr 17, 2020
Comment on lines +20 to +21
GC.Collect();
GC.WaitForPendingFinalizers();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
GC.WaitForPendingFinalizers();
if (tcs.Task.IsCompleted)
{
return;
}

That way we can exit early if the task finishes early.

@BrennanConroy
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@davidfowl davidfowl merged commit 812dfb9 into master Apr 18, 2020
@davidfowl davidfowl deleted the davidfowl/timer-rooting branch April 18, 2020 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants