cli: fix use of canceled connection in init
#89059
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
The implementation of `./cockroach init` first dials up the node (in a retry loop) and issues a `Health` RPC, to reduce the likelihood of accidental double-init of the cluster in case of network issues. However, this was trying to be too clever: if it managed to dial the node and check its health (again, using a short-lived context), it would then return that same connection to the caller for use in the Bootstrap RPC. Unfortunately, that connection would sit on top of an `rpc.Context` whose life was tied to a context[^1] that descended from one wrapped by `RunWithTimeout`. In other words, the context would be cancelled by the time the health check method returned. This seems to not cause issues today, since the `rpc.Context` seems to ignore this context cancellation. But in cockroachdb#88625, this suddenly became a problem and now that I've looked at this code, might as wel fix it regardless of whether cockroachdb#88625 is ever going to land. No release note since today's code happens to work. [^1]: https://github.com/cockroachdb/cockroach/blob/aecc58f125ac611f5793101cbd0323df569369db/pkg/cli/rpc_client.go#L46 Release note: None
|
This change is |
erikgrinaker
approved these changes
Sep 30, 2022
|
bors r=erikgrinaker |
|
Build succeeded: |
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.
The implementation of
./cockroach initfirst dials up the node (in aretry loop) and issues a
HealthRPC, to reduce the likelihood ofaccidental double-init of the cluster in case of network issues.
However, this was trying to be too clever: if it managed to dial the
node and check its health (using a short-lived context), it would
then return that same connection to the caller for use in the Bootstrap
RPC. Unfortunately, that connection would sit on top of an
rpc.Contextwhose life was tied to a context1 that descended from one wrapped by
RunWithTimeout. In other words, the context would be cancelled by thetime the health check method returned.
This seems to not cause issues today, since the
rpc.Contextseems toignore this context cancellation. But in #88625, this suddenly became
a problem and now that I've looked at this code, might as wel fix it
regardless of whether #88625 is ever going to land.
No release note since today's code happens to work.
Release note: None
Footnotes
https://github.com/cockroachdb/cockroach/blob/aecc58f125ac611f5793101cbd0323df569369db/pkg/cli/rpc_client.go#L46 ↩