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

context: relax recommendation against putting Contexts in structs #22602

Open
cespare opened this issue Nov 6, 2017 · 7 comments
Open

context: relax recommendation against putting Contexts in structs #22602

cespare opened this issue Nov 6, 2017 · 7 comments
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cespare
Copy link
Contributor

cespare commented Nov 6, 2017

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 #14660 (comment).

/cc @Sajmani @bradfitz @rsc

@cespare cespare added this to the Go1.11 milestone Nov 6, 2017
@Sajmani
Copy link
Contributor

Sajmani commented Nov 16, 2017

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
Copy link
Contributor Author

cespare commented Nov 16, 2017

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
Copy link
Contributor

Sajmani commented Nov 21, 2017 via email

@bradfitz bradfitz changed the title context: Relax recommendation against putting Contexts in structs context: relax recommendation against putting Contexts in structs Jun 13, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 14, 2018
@mvdan
Copy link
Member

mvdan commented Oct 17, 2018

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
Copy link
Contributor

as commented Oct 28, 2019

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

alok87 added a commit to practo/tipoca-stream that referenced this issue Feb 18, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Feb 18, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Jun 5, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Jun 5, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Jun 7, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Jun 7, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Jun 17, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
alok87 added a commit to practo/tipoca-stream that referenced this issue Jun 17, 2021
Had to go around this problem using mainContext as sessions get closed quite often when the total topics is huge.
Using context in struct instead of golang suggested approach of passing context around. Since sarama does not support passing context around.

IBM/sarama#1776
golang/go#22602
@dontlaugh
Copy link

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
Copy link
Contributor Author

cespare commented Oct 24, 2022

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.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants