Skip to content

Deflake TimeAwaitable finalizer test #21523

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 3 commits into from
May 6, 2020
Merged

Deflake TimeAwaitable finalizer test #21523

merged 3 commits into from
May 6, 2020

Conversation

BrennanConroy
Copy link
Member

Fixes #21517

I'm not really sure who was rooting this. I'm guessing some async state machine stuff since we do _ = new ObjectWithTimerAwaitable(tcs).Start(); so there could be a race where it's still rooted while we're running the GC

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label May 5, 2020
@BrennanConroy BrennanConroy requested a review from davidfowl May 5, 2020 22:23
@@ -19,8 +19,10 @@ public void FinalizerRunsIfTimerAwaitableReferencesObject()
var tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
UseTimerAwaitableAndUnref(tcs);

var timeout = tcs.Task.OrTimeout();
Copy link
Member

Choose a reason for hiding this comment

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

Below, we should be verifying Assert.True(tcs.Task.IsCompletedSuccessfully); to verify it didn't timeout.

I wonder why we need the while loop here and not in Kestrel's HttpConnectionManagerTests. Is it something about TimerAwaitable? I think the real fix might be to set the TimerAwaitable's dueTime to a very large value so you don't risk calling GC.Collect() while the timer is executing the callback.

// Create HttpConnection in inner scope so it doesn't get rooted by the current frame.
UnrootedConnectionsGetRemovedFromHeartbeatInnerScope(connectionId, httpConnectionManager, trace);
GC.Collect();
GC.WaitForPendingFinalizers();
var connectionCount = 0;
httpConnectionManager.Walk(_ => connectionCount++);
Assert.Equal(0, connectionCount);
trace.Verify(t => t.ApplicationNeverCompleted(connectionId), Times.Once());

Also, why is this test tracking _count? If anything I would check that Start() never finishes meaning the _timer is never disposed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Below, we should be verifying Assert.True(tcs.Task.IsCompletedSuccessfully); to verify it didn't timeout.

Whoops.

I wonder why we need the while loop here and not in Kestrel's HttpConnectionManagerTests.

That test isn't doing anything async it looks like. I'm guessing that an async state machine is rooting the object so there is a race

Copy link
Member

@halter73 halter73 May 6, 2020

Choose a reason for hiding this comment

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

I answered my own question. I think the real reason is we're "calling GC.Collect() while the timer is executing the callback." That's why I suggesting making the dueTime really large instead of adding this loop.

// Make sure it *really* cleans up
for (int i = 0; i < 5 && !tcs.Task.IsCompleted; i++)
while (!timeout.IsCompleted)
Copy link
Member

Choose a reason for hiding this comment

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

I kinda want to get rid of the loop to prove the dueTime theory.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Feel free to complain about me asking to remove the loop if it continues to be flaky. I just gotta know 😬

@BrennanConroy
Copy link
Member Author

I ran it 10000 times without the loop and it's reliable now. Before it would fail with less than 1000 runs.

@BrennanConroy BrennanConroy merged commit c9cdc48 into master May 6, 2020
@BrennanConroy BrennanConroy deleted the brecon/finalize branch May 6, 2020 02:45
@BrennanConroy BrennanConroy added this to the 5.0.0-preview5 milestone May 6, 2020
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.

[SignalR] Investigate failure in TimerAwaitableTests
2 participants