Skip to content

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
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

3 participants