all: document uses of context.Background by APIs

[matttproud]

Objective

Amend all non-root context.Background() use in the standard distribution to context.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

Background returns a non-nil, empty Context. It is never canceled, has no values, and has no deadline. It is typically used by the main function, initialization, and tests, and as the top-level Context for incoming requests.

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:

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).

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.

// This is almost entirely inconceivable.
if ctx == context.Background() {

There are approximately 53 non-test, non-vendor usages of context.Background in /usr/local/go for go version go1.15.8 linux/amd64:

$ grep -r 'context.Background()' | grep -v -E '(testdata|^vendor|_test\.go)' | wc -l
53

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 eight context.Background calls are probably excusable.

Proposed Solution

Audit calls to context.Background and swap them to context.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:

type Server struct {
  db *sql.DB
}

func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
  ctx, cancel := context.WithCancel(r.Context())  // implied that middleware may deadline
  defer cancel()

  rows, err := s.DB.Query(q)  // extremely like the user would want this to be canceable
}

Internally (*sql.DB).Query calls (*sql.DB).QueryContext with context.Background. A user wanting cancellation would use (*sql.DB).QueryContext.

Or in the case of gRPC (with code supra):

func (s *Server) Echo(ctx context.Context, req *echopb.EchoRequest) (*echopb.EchoResponse, error) {

  // store echo queries in an audit log to satisfy the auditors
  rows, err := s.DB.Query(q)  // extremely like the user would want this to be canceable
}

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), see context.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.

[matttproud]

/CC: @jadekler (apropos the text we are working on)

[seankhliao]

I don't think it makes too much sense to do this for the stdlib in the approx 60 cases where context.Background() appears in non test code. These are almost all used where the exposed API doesn't take a context

• there is no ambiguity in which context to use, because none are available and the user explicitly asked for a blocking function
• it can't be "not yet available" because of the compatibility promise

[ianlancetaylor]

CC @Sajmani

[bcmills]

it can't be "not yet available" because of the compatibility promise

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 http.NewRequest vs. http.NewRequestWithContext.)

So in that sense it really is a TODO — either the user needs to call something else to fill in the context (such as (*net/http.Request).WithContext), or the API needs to be augmented and/or deprecated.

[jeanbza]

FWIW this seems to me like good way to lead by example wrt how context.TODO should be used, an easy (if a bit toilsome) change, and no downsides.

[Sajmani]

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.

[matttproud]

I have been mulling a smaller proposal to add a section to the Godoc entry for package context to explain the retrofitting techniques, what context.TODO means in the context of retrofitting, etc. As a high-level sketch, do you think that would satisfy the desire for actionable guidance?

[matttproud]

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.

[Sajmani]

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 notifications@github.com 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 https://github.com/golang/go/issues/44143#issuecomment-776277807, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACKIVXOYO237TIQ7GIHC7MDS6GXC7ANCNFSM4XHPHPQA .

[bcmills]

Callers of Load can incrementally be upgraded to LoadContext.

See also #32816.

[rsc]

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?

[rsc]

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

[rsc]

Discussed this with @mattproud a bit last week. It sounds like the main thing we need to do is to document clearly which routines in the standard library use context.Background internally (and will always do that).

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.

[rsc]

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. The kind of comment I imagine would be:

// Dial uses context.Background internally; to specify the context, use DialContext.

[rsc]

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

[matttproud]

I'll take a stab at this very soon. Haven't forgotten.

[gopherbot]

Change https://golang.org/cl/296152 mentions this issue: docs: clarify when APIs use context.Background.

[matttproud]

I had been thinking a lot about https://github.com/golang/go/issues/44143#issuecomment-776938138, and it was referenced in another context (no pun intended). My thoughts are roughly this:

Enclosing context.Background in non-program roots should be avoided (especially for retrofitting), because it insinuates that casual use of context.Background in non-roots is permissible, especially in library code. This remains significant enough problem in code review that we even codified guidance about it in local style guide documentation. I want to avoid the appeal-to-authority from because-the-standard-library-does-it-I-can-do-it-even-though-I-probably-misdesigned-my-library-in-other-ways-alluded-to-below problem. context.TODO evokes a stop-and-think response.

While the implementation of the two contexts is the same, they do convey different information. context.Background is permanent fixture of program roots, whereas context.TODO decisively indicates that something is a transitionary state to be made better later (when is another question[0]). Rather what we should prefer is having APIs let the user specify the context.Context to be used, for many of the same reasons as this article alludes to.

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:

1. What's derived therefrom is always interruptible/cancelable by other means and exposed API? This entails no goroutine leaking.

2. Callers of that type to express cancellation or deadline of their own.

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.

[matttproud]

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.

[gopherbot]

Change https://golang.org/cl/301069 mentions this issue: net/http: revert change to generated file from CL 296152