context: relax recommendation against putting Contexts in structs

[cespare]

This is a follow-on from discussion in #14660.

Right now the context package documentation says

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx: [...]

This advice seems overly restrictive. @bradfitz wrote in that issue:

While we've told people not to add contexts to structs, I think that guidance is over-aggressive. The real advice is not to store contexts. They should be passed along like parameters. But if the struct is essentially just a parameter, it's okay. I think this concern can be addressed with package-level documentation and examples.

Let's address this concern with package-level documentation.

Also see @rsc's comment at https://github.com/golang/go/issues/14660#issuecomment-200145301.

/cc @Sajmani @bradfitz @rsc

[Sajmani]

I'm open to relaxing this restriction as long as we can clearly define what a parameter struct is. However, I want to avoid having people define custom wrappers around Context like FooContext; I think that will lead to a world of hurt when a function that takes FooContext wants to call one that takes BarContext. The current restriction prevents people from using the type system to create this kind of impedance mismatch.

I had also pushed for the explicit parameter restriction so that we could more easily automate refactorings to plumb context through existing code. But seeing as we've failed to produce such tools, we should probably loosen up here and deal with the tooling challenges later.

[cespare]

Hey @Sajmani, one of the reasons this came up is that at our company we are (I think) doing exactly this:

However, I want to avoid having people define custom wrappers around Context like FooContext;

We have a database system that defines a type

type queryContext struct {
    ctx context.Context
    // Other fields here are mostly shared state, some of which is
    // query-specific (including counters and other stats for this query)
    // and some of which is shared between queries (such as a semaphore that
    // bounds the parallelism of CPU-intensive work).
}

This has worked well for us. The context.Context is useful for propagating timeouts/cancelation throughout the query goroutines (and across the whole distributed system).

In our system, I don't see how this applies:

I think that will lead to a world of hurt when a function that takes FooContext wants to call one that takes BarContext.

Our queryContext type is local to the package; APIs that access this database take context.Context as the first argument. Internal functions take a *queryContext as the first argument. Where we call standard library functions or third-party APIs, we pass along qx.ctx. There is no impedance mismatch.

So, two questions:

1. Is your objection to the FooContext specifically about writing exported APIs that take a FooContext argument rather than a context.Context? Perhaps that is the thing we should recommend against? (Or have I misunderstood your objection?)

2. If we wanted to avoid nesting a context.Context inside our own struct, we would probably have to change dozens of internal methods from

   func (t *T) doAThing(qx *queryContext, ...)

   to

   func (t *T) doAThing(ctx context.Context, qx *queryContext, ...)

   Would you recommend we do that? It frankly seems like too much context to me.

[Sajmani]

Yes, the impedance mismatch is primarily a concern for exported types.

The scenario I'm worried about is:

1. package foo defines type Context that contains a context.Context and, say, method Foo() *Foo, for accessing foo-specific data.
2. package bar defines type Context that contains a context.Context and, say, method Bar() *Bar, for accessing bar-specific data.
3. Some function that accepts a foo.Context needs to call a bar.Context. Passing just the context.Context loses the Foo and doesn't necessarily know what to pass for Bar:

func F(fctx foo.Context) {
  bctx := bar.NewContext(fctx.Context(), nil /*the *Bar value*/)
  G(bctx)
}

Furthermore, if G later calls a function that expects a foo.Context, the *Foo that should have been plumbed through has been lost:

func G(bctx bar.Context) {
  fctx := foo.NewContext(bctx.Context(), nil /*the *Foo value*/)
  H(fctx)
}

The point of the context.Context.Value design is that packages foo and bar don't care about any of this. Their values propagate through layers of the call stack without intersecting or interfering with each other. This is exactly what you want for things like trace IDs, authentication scopes, profiling tags. But this is not what you should use for API-visible options.

The "keep Context out of structs" restriction is a simple rule to follow that helps users avoid problems like this, but it is overly restrictive. The real guidelines are harder to articulate, but perhaps we should try harder to do so. S

[mvdan]

I was also thrown off by the recommendation in the package godoc. I agree that it should be fine to pass a context inside a struct as long as the function isn't exported. For my own purposes, it lets me avoid some boilerplate - the second option below is clearer, and avoids repetition in a number of signatures.

// separate context
func expandSomething(ctx context.Context, ectx expandContext, format string, a ...interface{}) string
// part of expandContext
func expandSomething(ectx expandContext, format string, a ...interface{}) string

So I think the recommended restriction should be modified to only apply to exposed APIs, i.e. exported functions. All the disadvantages outlined above generally don't apply to unexported funcs.

[as]

@mvdan I do not fully agree with this because I feel like it misses an important improvement to an exported API.

The context.Context is big and ugly, and its requirement to pass it as the first parameter implies one of the following:

• Function signatures must all have context.Context as their first parameter
• Functions must be written twice

Both of these make things significantly more annoying, and we have issues where context.Context is used as an argument for parametric polymorphism, goroutine-local storage, and other interesting features. I believe that these stem from the recommendation against object-local storage for request scoped context.Context values, and that a request scoped object could sufficiently provide a means to decorate a request with a context.Context for requests that actually want to use it.

Having written that, an potentially important pitfall is raised by @Sajmani about the loss of the original context.Context if it resides in a struct:

func F(fctx foo.Context) {
  bctx := bar.NewContext(fctx.Context(), nil /*the *Bar value*/)
  G(bctx)
}
Furthermore, if G later calls a function that expects a foo.Context, the
*Foo that should have been plumbed through has been lost:

I believe that if we are to relax the recommendation for unexported functions, we can also argue that the body of an exported function is under the domain of the API-author's control too, and should also be relaxed. Therefore, the API author should be responsible for deciding whether or not the new context is necessary for the request and how it is passed down to descendants: to use a request scoped object with a chain of method calls, the context.Context as the first parameter, or a value-duplication of the original request scoped object followed by a method call on that object.

[dontlaugh]

I've taken to adding contexts as private fields on some higher-level manager/application/orchestration type objects, and re-deriving sub-contexts where appropriate.

I essentially do a version of what this reddit user says https://www.reddit.com/r/golang/comments/lro5va/contexts_and_structs/gomy4ip

[cespare]

Using errgroup is another case where I think it's totally normal and fine to store contexts inside structs. With errgroup, a context may not be used for a "request", necessarily, but purely as a cancellation mechanism for an arbitrary collection of tasks. If those tasks are associated with a struct, it sometimes makes sense to store the context and/or the CancelFunc in that struct.

(Perhaps you could argue that this is a misuse of context in the first place, but errgroup has established this pattern pretty firmly at this point. And in general it works fine apart from the confusion wrt the "Do not store contexts in structs" advice.)

At this point, after several years of using and reviewing context-using code, I've never yet reviewed code that misuses contexts in the way that the documentation warns against. (I'm sure it happens, particularly at Google scale, but I'm starting to think that it's not that common.) On the other hand, I've seen a lot of confusion and discussions arising from the "Don't store contexts in structs" advice and encountered multiple situations in which I concluded that it was fine, in fact, to store a context in a struct in some particular case. So in my opinion completely deleting this part of the documentation would be an improvement over the status quo.