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 · 5 comments
Open

context: relax recommendation against putting Contexts in structs #22602

cespare opened this issue Nov 6, 2017 · 5 comments

Comments

@cespare
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

@Sajmani Sajmani commented Nov 21, 2017

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.