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

go1.21: use context.WithoutCancel to avoid propagating user-initiated cancelations #5506

Closed
aarongable opened this issue Jun 28, 2021 · 3 comments · Fixed by #7073
Closed
Assignees

Comments

@aarongable
Copy link
Contributor

As of #5459, requests which arrive at the SA have their ctx cleared and replaced after the SA starts processing the request. This is to work around an issue where context cancellation (e.g. due to a deadline/timeout coming up, or due to the initiating gRPC request being closed) propagating into sql query cancellation causes sql connections to be marked as bad and dropped from the pool, resulting in connection churn.

However, this prevents the context object from carrying trace information as it moves through the SA; the SA cannot add custom trace data or additional spans, because the context object has been cleared of that data.

We should investigate other ways to prevent cancellation of sql queries that would allow the incoming context object to be propagated throughout the SA to a greater degree.

@aarongable
Copy link
Contributor Author

When go1.21 or go1.22 is released, we should use the upcoming context.Detach API to accomplish this.

@jsha
Copy link
Contributor

jsha commented Mar 15, 2023

is to work around an issue where context cancellation (e.g. due to a deadline/timeout coming up, or due to the initiating gRPC request being closed) propagating into sql query cancellation causes sql connections to be marked as bad and dropped from the pool, resulting in connection churn.

Note that this impact on connection churn is still hypothesized - we haven't yet been able to prove that this is a cause of connection churn.

Also, an update: we now "detach" contexts at the WFE instead of the SA.

@jsha jsha changed the title Try to propagate context further into the SA Use context.Detach API to avoid propagating user-initiated cancelations Mar 15, 2023
@aarongable
Copy link
Contributor Author

aarongable commented Jun 21, 2023

go1.21 is shipping with context.WithoutCancel! https://tip.golang.org/doc/go1.21

We may also be able to handle some of our use-cases by using the new context.AfterFunc to register cleanup functions that should be run after the context is canceled.

@aarongable aarongable changed the title Use context.Detach API to avoid propagating user-initiated cancelations go1.21: use context.WithoutCancel to avoid propagating user-initiated cancelations Jun 21, 2023
@aarongable aarongable self-assigned this Sep 8, 2023
@aarongable aarongable added this to the Sprint 2023-09-05 milestone Sep 8, 2023
aarongable added a commit that referenced this issue Sep 11, 2023
This new standard library method returns a context with all of the
original metadata (e.g. tracing spans) still attached, but which will
not be canceled by any cancel funcs, deadlines, or timeouts set on the
parent context. We do this manually in a few places to prevent client
cancellations (usually disconnects) from disrupting our work, so this
just makes that code slightly simpler.

Fixes #5506
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 a pull request may close this issue.

2 participants