refactor(enginenetx): make LookupTactics async #1300
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than waiting for LookupTactics to complete, make it async and let it stream the tactics back to the caller. This design allows us to start connection while DNS lookups are still in progress when we have configured beacons for specific hosts. In turn, this means we could perform more operations in the same unit of time, by overallapping some operations. Additionally, the new design would also work quite well with a DNS resolver that awaits for additional responses after the first. While there, recognize that the HTTPSDialer code and the code in the related structs was a bit more complex than it could. This diff rewrites code to mostly pass the context around and only honour it in network-like operations (e.g., when waiting to be ready, performing DNS lookup, TCP dialing, or TLS handshaking). The rest of the infrastructure around the code that performs network operations always assumes to read channels until completion and to emit all the possible events. So, readers will read as much as they can and writes will write as much as they can but canceling the context will cause network operations to be interrupted and hence the reporting of many errors. Part of ooni/probe#2531
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
Rather than waiting for LookupTactics to complete, make it async and let it stream the tactics back to the caller. This design allows us to start connecting while DNS lookups are still in progress when we have configured beacons for specific hosts. In turn, this means we could perform more operations in the same unit of time, by overalapping some DNS lookups and TCP+TLS dials. Additionally, the new design would also work quite well with a DNS resolver that awaits for additional responses after the first one and returns all of them as tactics. While there, recognize that the HTTPSDialer code and the code in the related structs was a bit more complex than it should be. We don't need to explicitly honor the context when moving data between goroutines as long as the writer goroutines write until completion and then close the channel, and as long as reader goroutines read until either the channel is closed (when there's a single writer) or all the possible writers have completed (otherwise). Networking code and networking-like code is the only code that MAY block and for which we really need a context. With the new simplified design, all the goroutines will join before `DialTLSContext` returns, hence we don't need anymore a `sync.WaitGroup` to make sure we're not leaking any goroutine in this code. Part of ooni/probe#2531
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than waiting for LookupTactics to complete, make it async and let it stream the tactics back to the caller. This design allows us to start connecting while DNS lookups are still in progress when we have configured beacons for specific hosts. In turn, this means we could perform more operations in the same unit of time, by overalapping some DNS lookups and TCP+TLS dials. Additionally, the new design would also work quite well with a DNS resolver that awaits for additional responses after the first one and returns all of them as tactics.
While there, recognize that the HTTPSDialer code and the code in the related structs was a bit more complex than it should be. We don't need to explicitly honor the context when moving data between goroutines as long as the writer goroutines write until completion and then close the channel, and as long as reader goroutines read until either the channel is closed (when there's a single writer) or all the possible writers have completed (otherwise). Networking code and networking-like code is the only code that MAY block and for which we really need a context.
With the new simplified design, all the goroutines will join before
DialTLSContextreturns, hence we don't need anymore async.WaitGroupto make sure we're not leaking any goroutine in this code.Part of ooni/probe#2531