-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Conversation
@@ -19,8 +19,10 @@ public void FinalizerRunsIfTimerAwaitableReferencesObject() | |||
var tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously); | |||
UseTimerAwaitableAndUnref(tcs); | |||
|
|||
var timeout = tcs.Task.OrTimeout(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 😬
I ran it 10000 times without the loop and it's reliable now. Before it would fail with less than 1000 runs. |
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