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

Move code from RealConnection to RealConnectPlan #7065

Merged
merged 3 commits into from
Feb 10, 2022

Conversation

swankjesse
Copy link
Collaborator

Also promote RealConnectPlan to a top-level type, ConnectPlan.

Each RealConnection is now created only once its ready to be used
to carry exchanges. We do the actual connect work in ConnectPlan.

Also promote RealConnectPlan to a top-level type, ConnectPlan.

Each RealConnection is now created only once its ready to be used
to carry exchanges. We do the actual connect work in ConnectPlan.
@yschimke
Copy link
Collaborator

yschimke commented Feb 8, 2022

Tests taking 4 hours suggests an issue.

Overall looks like a sensible refactoring of RealConnection.

@swankjesse swankjesse marked this pull request as ready for review February 9, 2022 12:57
@swankjesse
Copy link
Collaborator Author

The first timeouts were a basic blunder; I didn't hand off the HTTP2 connection object from ConnectPlan to RealCinnection.

The next problem was I didn't fix FastFallbackExchangeFinder to cycle through the retry plans when those were returned.

With those fixed, tests pass.

I saw some tests flake but I think it's a test fragility problem and not a correctness problem. In particular FastFallbackExchangeFinderTest assumes IPv6 will complete first, which is likely but not guaranteed. I don't think this is a new flake in this PR. But it'll need some work.

Next step is splitting ConnectPlan into two steps: TCP handshake and then everything else. With that I should be able to promote the TCP handshake to be the only subject of the race.

@swankjesse swankjesse merged commit 699a212 into master Feb 10, 2022
@swankjesse
Copy link
Collaborator Author

Working towards #506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants