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

Fix flakey HTTP/2 tests #39588

Merged
merged 3 commits into from
Jan 19, 2022
Merged

Fix flakey HTTP/2 tests #39588

merged 3 commits into from
Jan 19, 2022

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 17, 2022

Addresses #39520

@JamesNK
Copy link
Member Author

JamesNK commented Jan 17, 2022

Might also address #39477 & #39479 & #39492

These all started failing at the same time.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 18, 2022

Could I get a review? 🙏 I'd like to merge this to stop any more flakey tests from popping up from this issue.

@BrennanConroy
Copy link
Member

Could you explain how this fixes the issue?

@JamesNK
Copy link
Member Author

JamesNK commented Jan 18, 2022

I talked with @halter73 when he made this change. I believe awaiting ThreadPoolAwaitable.Instance before sending preface and settings means the test loses the SynchronizationContext so the Http2Connection request processing runs inline.

The problem that this PR fixes is ThreadPoolAwaitable.Instance was nested in a method. When returning from that method the context is re-acquired before SendPreface/SendSettings. Moving ThreadPoolAwaitable.Instance so it is in a method before SendPreface & SendSettings fixes this.

Correct me if I'm wrong @halter73 😄

@halter73
Copy link
Member

Awaiting ThreadPoolAwaitable.Instance should lose the current SynchronizationContext even for callers several async methods up stack.

It's different than ConfigureAwait(false) because it's not just returning a custom awaitable that doesn't restore the sync context. It's also posting the continuation using ThreadPool.UnsafeQueueUserWorkItem which loses the entire execution context. Up stack callers may still try to restore the sync context from the execution context, but there shouldn't be any sync context to restore.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 18, 2022

This PR definitely fixes the flakey test. Before 5000 executions would result in 30ish failures. After 5000 executions all pass.

Not having ThreadPoolAwaitable.Instance in a nested method has definitely had an impact.

@halter73
Copy link
Member

I was wrong with my last comment. Task captures the ExecutionContext when it's constructed so it uses that ExecutionContext rather than the ExecutionContext of the thread it's completed on (thankfully thinking about this more).

This makes me want to return a ConfiguredTaskAwaitable from InitializeConnectionAsync so the entire tests run without the sync context like I thought was happening before, but it doesn't have to be this PR. E.g.

protected ConfiguredTaskAwaitable InitializeConnectionAsync(RequestDelegate application, int expectedSettingsCount = 3, bool expectedWindowUpdate = true)
{
    return InitializeConnectionInnerAsync(application, expectedSettingsCount, expectedWindowUpdate).DefaultTimeout().ConfigureAwait(false);
}

@JamesNK JamesNK merged commit 7f67c1f into main Jan 19, 2022
@JamesNK JamesNK deleted the jamesnk/http2-flaky-tests branch January 19, 2022 02:44
@ghost ghost added this to the 7.0-preview1 milestone Jan 19, 2022
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
@halter73 halter73 mentioned this pull request Apr 13, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants