-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
all: document uses of context.Background by APIs #44143
Comments
/CC: @jadekler (apropos the text we are working on) |
I don't think it makes too much sense to do this for the stdlib in the approx 60 cases where
|
CC @Sajmani |
Maybe? But for many of those cases we can deprecate the old API and provide a parallel new API that does accept a context. (Consider So in that sense it really is a |
FWIW this seems to me like good way to lead by example wrt how |
I like this idea, but I'd want to know how this will turn into actionable guidance for the user. For example, suppose we had a tool to notice when a function that has a context parameter transitively calls context.TODO(). In this case, the user likely wants to "plumb" its context down to the context.TODO(). As @bcmills points out, this probably means switching some of the intervening function calls to the versions that take a context parameter. This seems like a tractable problem (and IIRC people have also tried creating automatic refactoring tools to do context plumbing), but what I want to avoid is confusion for users reading this code or getting notice about transitively-reachable context.TODO() without guidance on what they should do instead. |
I have been mulling a smaller proposal to add a section to the Godoc entry for package context to explain the retrofitting techniques, what |
Here is what I have come up (as a sketch) for the documentation for retrofitting: // TODO returns a non-nil, empty Context. Code should use context.TODO when
// it's unclear which Context to use or it is not yet available (because the
// surrounding function has not yet been extended to accept a Context
// parameter).
//
// Retrofitting Context Support onto APIs
//
// Retrofitting means adding a context to an API that otherwise did not
// originally have support for it. Suppose we want to retrofit a function
// called Load:
//
// // Load reads data from the network.
// func Load() ([]byte, error) {
// // ...
// }
//
// Updating a function usually requires updating its callers. This means a new
// API should be introduced that supports it. Conventionally it is the original
// name with the word "Context" appended to its end.
//
// // LoadContext reads data from the network subject to context
// // cancellation.
// func LoadContext(ctx context.Context) ([]byte, error) {
// // ... use ctx ...
// }
//
// Afterwards the old Load function should be updated to call the new
// LoadContext function. When Load does, it should pass context.TODO()
// for the context argument.
//
// // Load reads data from the network.
// func Load() ([]byte, error) {
// return LoadContext(context.TODO())
// }
//
// Callers of Load can incrementally be upgraded to LoadContext. If the callers
// do not have access to a context already, they should use context.TODO(). The
// retrofitting process continues until the APIs reach a function with a
// suitably-scoped context not derived from context.TODO().
//
// Maintainers may optionally deprecate the old APIs that they have
// retrofitted.
//
// // Load reads data from the network.
// //
// // Deprecated: Please use LoadContext instead and provide a context.
// func Load() ([]byte, error) {
// return LoadContext(context.TODO())
// }
func TODO() Context {
return todo
} I put modest work into the text above and am not particularly attached to its exact phrasing. |
I think it's worth adding some explanatory text like this about
context.TODO; I have some suggestions*, but it's directionally correct. I'm
not sure whether this text belongs in godoc or in a separate reference doc
linked from there.
*The example function should have some parameters, so it's clear Context is
conventionally added as the first parameter. Also, we should make it clear
that the FooContext variant naming should only be used when retrofitting;
people shouldn't include Context in the name of functions just because they
take a context parameter.
…On Tue, Feb 9, 2021 at 5:09 PM Matt T. Proud ***@***.***> wrote:
Here is what I have come up (as a sketch) for the documentation for
retrofitting:
// TODO returns a non-nil, empty Context. Code should use context.TODO when// it's unclear which Context to use or it is not yet available (because the// surrounding function has not yet been extended to accept a Context// parameter).//// Retrofitting Context Support onto APIs//// Retrofitting means adding a context to an API that otherwise did not// originally have support for it. Suppose we want to retrofit a function// called Load://// // Load reads data from the network.// func Load() ([]byte, error) {// // ...// }//// Updating a function usually requires updating its callers. This means a new// API should be introduced that supports it. Conventionally it is the original// name with the word "Context" appended to its end.//// // LoadContext reads data from the network subject to context// // cancellation.// func LoadContext(ctx context.Context) ([]byte, error) {// // ... use ctx ...// }//// Afterwards the old Load function should be updated to call the new// LoadContext function. When Load does, it should pass context.TODO()// for the context argument.//// // Load reads data from the network.// func Load() ([]byte, error) {// return LoadContext(context.TODO())// }//// Callers of Load can incrementally be upgraded to LoadContext. If the callers// do not have access to a context already, they should use context.TODO(). The// retrofitting process continues until the APIs reach a function with a// suitably-scoped context not derived from context.TODO().//// Maintainers may optionally deprecate the old APIs that they have// retrofitted.//// // Load reads data from the network.// //// // Deprecated: Please use LoadContext instead and provide a context.// func Load() ([]byte, error) {// return LoadContext(context.TODO())// }func TODO() Context {
return todo
}
I put modest work into the text above and am not particularly attached to
its exact phrasing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44143 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACKIVXOYO237TIQ7GIHC7MDS6GXC7ANCNFSM4XHPHPQA>
.
|
See also #32816. |
Many of the uses of context.Background could in theory change to context.TODO, but we are never going to "DO" those changes. It will make it look like there's work we can do that we can't. (For example Dial vs DialContext.) I'm not sure I see what benefit there is in making the code look like there is work to do when there isn't. I believe that all these routines are documented as using the background context. Maybe it would make sense to proceed with clearer docs instead? |
This proposal has been added to the active column of the proposals project |
Discussed this with @mattproud a bit last week. There may be room for a doc explaining how to use TODO in a migration adding contexts, but that might be a separate doc rather than a long doc comment in package context. |
At this point, what's left doesn't need to go through the proposal process. It's fine to just send CLs for the comments.
|
No change in consensus, so accepted. 🎉 |
I'll take a stab at this very soon. Haven't forgotten.
… |
Change https://golang.org/cl/296152 mentions this issue: |
I had been thinking a lot about #44143 (comment), and it was referenced in another context (no pun intended). My thoughts are roughly this: Enclosing While the implementation of the two contexts is the same, they do convey different information. Even in the cases of RPC or HTTP request dispatching, I am still not 100 percent sure that I would consider those roots but rather branches of a central program root context. These branches often have request-scoped data and very well-defined lifetimes. And it would be wrong to attach request-scoped data to the root context (that itself is server/program lifetime scoped). This is a whole other topic I have been mulling. There are a number of interesting things the single-root approach enables: full program tracing, universal credential passing, etc. I've been failing to come up with a good litmus of when context.Background is really safe to use in libraries as non-roots. Brainstorm:
Classically there has been a risk of this going wrong in network resolution code like request balancers (e.g., gRPC) or name resolution where these libraries perform operations in the request path using an encapsulated background context. It's an error-prone pattern for experts. Then we have the question of whether arbitrary cancellation is necessarily safe and how APIs can be designed to either handle cleanup gracefully with it or when another mechanism through formal API is advisable. @jadekler and I had been in discussion around this topic as a potential new article or library authorship exercise to help explain how to do this safely. [0] — I was refereeing a set of ideas around "when" with respect to the compatibility guarantee. For instance, perhaps as a "Go 2" proposal, we replace the non-injectable context APIs in the standard library with injectable ones (assuming context still lives in its current form). Or potentially we could deprecate the non-injectable forms before "Go 2" and nudge folks toward the injectable ones with the "Deprecated: " documentation protocol. As you can see, I did not want to cloud an otherwise large proposal with something like this that could turn it into a kitchen sink of a proposal. |
Small nudge: do you think https://go-review.googlesource.com/c/go/+/296152 will be alright for the raw documentation part? I’d like to get that squared away before looking at next steps. |
The Go standard library retrofitted context support onto existing APIs using context.Background and later offered variants that directly supported user-defined context value specification. This commit makes that behavior clear in documentation and suggests context-aware alternatives if the user is looking for one. An example motivation is supporting code for use in systems that expect APIs to be cancelable for lifecycle correctness or load shedding/management reasons, as alluded to in https://blog.golang.org/context-and-structs. Updates #44143 Change-Id: I2d7f954ddf9b48264d5ebc8d0007058ff9bddf14 Reviewed-on: https://go-review.googlesource.com/c/go/+/296152 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Jean de Klerk <deklerk@google.com> Trust: Jean de Klerk <deklerk@google.com> Run-TryBot: Jean de Klerk <deklerk@google.com> TryBot-Result: Go Bot <gobot@golang.org>
Change https://golang.org/cl/301069 mentions this issue: |
This file is generated, so the fix needs to happen upstream. The file can then be regenerated using 'go generate net/http'. Updates #44143 Change-Id: I13a1e7677470ba84a06976e5bbe24f4ce1e7cfb2 Reviewed-on: https://go-review.googlesource.com/c/go/+/301069 Trust: Bryan C. Mills <bcmills@google.com> Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: David Chase <drchase@google.com> TryBot-Result: Go Bot <gobot@golang.org>
Objective
Amend all non-root
context.Background()
use in the standard distribution tocontext.TODO()
.A central purpose is to have the Go source code serve as a prime example of correct context usage to newcomers and better teach users how to retrofit support after-the-fact.
Definitions
A classical program’s roots are: func main, test functions, and func init. These are effectively the entrypoints where user code can begin outside of other top-level variable declarations.
Background
The context API debuted in 2014 and became part of the standard distribution in release 1.7.
Many APIs that were context-capable but predated the API retrofitted support with the introduction of function names suffixed with “Context” to indicate “this is the context-capable variant”. An example is
(*sql.DB).QueryRow
and(*sql.DB).QueryRowContext
. This is largely due to Go 1 compatibility guarantee, which meant backward compatibility could not be broken.The problem is users do not always respect the guidance associated with
context.Background
and instead use
context.Background
deep in call graphs instead of propagating a context from outward to the proper scope where the value is required. Where this is infeasible,context.TODO
is precisely the tool to use. To wit:If we amend the standard distribution, it effectively provides a solid affirmative use for
context.TODO
and helps reify the design counsel. This has several additional knock-on benefits for encouraging design of production-correct and -scalable APIs, which requires systems to correctly and predictably handle load and interruption (e.g., to avoid unbounded memory usage).Scale and Requirements
Replacement of the default context should introduce no behavior delta for existing users. Further, it should not regress performance. Neither of these should be a problem in practice with production code. It would be extremely unconventional for an end-user developer or infrastructure developer to care about context identity.
There are approximately 53 non-test, non-vendor usages of
context.Background
in/usr/local/go
forgo version go1.15.8 linux/amd64
:Outside of request-scoped contexts (no pun-intended) like
package http
used to initiate client calls or handle external requests, the above litmus test for root contexts likely fails for most of the standard distribution. About eightcontext.Background
calls are probably excusable.Proposed Solution
Audit calls to
context.Background
and swap them tocontext.TODO
where they clearly are not root operations nor could under any real rubric count as one. A litmus test:If any transitive function calls made by the following roots should be cancelable,
context.TODO
is more appropriate:Internally
(*sql.DB).Query
calls(*sql.DB).QueryContext
withcontext.Background
. A user wanting cancellation would use(*sql.DB).QueryContext
.Or in the case of gRPC (with code supra):
To be sure,
context.TODO
in the code above won’t miraculously become cancelable (that's not the point). But a curious Gopher may compare the two implementations (e.g.,(*sql.DB).Query
and(*sql.DB).QueryContext
), seecontext.TODO
, and then be prompted to consider how these semantics should fit into the design. And this is not about preventing or deprecating non-context-correct older public API variants in the standard distribution (though this has been something I’ve thought about a lot over the years).Alternatives Considered
While this defect report/proposal was motivated by incorrect code in user code, which is classically the purview of static analysis of something to report, I think it would be best if Go took its own medicine here by applying the advice correctly itself. ;) Appeals to authority happen, and the Go source tree has a semi-canonical status in the mindset of the user base.
The text was updated successfully, but these errors were encountered: