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

proposal: context: enable first class citizenship of third party implementations (case when custom context needs to create is own done channel) #36448

Open
navytux opened this issue Jan 8, 2020 · 6 comments

Comments

@navytux
Copy link
Contributor

@navytux navytux commented Jan 8, 2020

Hello up there.

Please find below description of real-world case where third party context needs to create its own done channel. @rsc was asking for example of such cases and I already commented on the subject on #28728 (comment), but it looks that feedback was lost due to that #28728 was already closed. Please either reopen #28728 or consider my case here. Thank you.

---- 8< ----
@rsc wrote at #28728 (comment):

There will still be a goroutine if WithCancel/WithDeadline are passed a parent context implementation that allocates its own done channels. I don't think that's particularly common, but of course I'd love to hear otherwise.

@rsc wrote at #28728 (comment):

are there any real-world examples of code that would still allocate goroutines in WithCancel etc?

I'm a bit late here, but below is example of real-world usage where custom context needs to create its own done channel: this need arises when one wants to merge two contexts to cancel the work when either the service is stopped, or when caller's context is canceled. Let me quote https://godoc.org/lab.nexedi.com/kirr/go123/xcontext#hdr-Merging_contexts to explain:

---- 8< ----

Merging contexts

Merge could be handy in situations where spawned job needs to be canceled whenever any of 2 contexts becomes done. This frequently arises with service methods that accept context as argument, and the service itself, on another control line, could be instructed to become non-operational. For example:

func (srv *Service) DoSomething(ctx context.Context) (err error) {
	defer xerr.Contextf(&err, "%s: do something", srv)

	// srv.serveCtx is context that becomes canceled when srv is
	// instructed to stop providing service.
	origCtx := ctx
	ctx, cancel := xcontext.Merge(ctx, srv.serveCtx)
	defer cancel()

	err = srv.doJob(ctx)
	if err != nil {
		if ctx.Err() != nil && origCtx.Err() == nil {
			// error due to service shutdown
			err = ErrServiceDown
		}
		return err
	}

	...
}

func Merge

func Merge(parent1, parent2 context.Context) (context.Context, context.CancelFunc)

Merge merges 2 contexts into 1.

The result context:

  • is done when parent1 or parent2 is done, or cancel called, whichever happens first,
  • has deadline = min(parent1.Deadline, parent2.Deadline),
  • has associated values merged from parent1 and parent2, with parent1 taking precedence.

Canceling this context releases resources associated with it, so code should call cancel as soon as the operations running in this Context complete.

---- 8< ----

To do the merging of ctx and srv.serveCtx done channels current implementation has to allocate its own done channel and spawn corresponding goroutine:

https://lab.nexedi.com/kirr/go123/blob/5667f43e/xcontext/xcontext.go#L90-118
https://lab.nexedi.com/kirr/go123/blob/5667f43e/xcontext/xcontext.go#L135-150

context.WithCancel, when called on resulting merged context, will have to spawn its own propagation goroutine too.

For the reference here is context.Merge implementation in Pygolang that does parents - child binding via just data:

https://lab.nexedi.com/kirr/pygolang/blob/64765688/golang/context.cpp#L74-76
https://lab.nexedi.com/kirr/pygolang/blob/64765688/golang/context.cpp#L347-352
https://lab.nexedi.com/kirr/pygolang/blob/64765688/golang/context.cpp#L247-251
https://lab.nexedi.com/kirr/pygolang/blob/64765688/golang/context.cpp#L196-226

Appologize, once again, for being late here, and thanks beforehand for taking my example into account.
Kirill

/cc @Sajmani

@toothrot toothrot added this to the Backlog milestone Jan 8, 2020
@toothrot toothrot changed the title Fwd: proposal: context: enable first class citizenship of third party implementations (case when custom context needs to create is own done channel) proposal: context: enable first class citizenship of third party implementations (case when custom context needs to create is own done channel) Jan 8, 2020
@gopherbot gopherbot added the Proposal label Jan 8, 2020
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 10, 2020

Note that it doesn't actually need to allocate its own Done channel, so you can still avoid extra goroutines for child contexts. But it's true that you do still need an extra goroutine for the Merge context itself if more than one parent is cancelable.

Consider: https://play.golang.org/p/w-EniGk-qcH

@bcmills bcmills modified the milestones: Backlog, Proposal Jan 10, 2020
@navytux

This comment has been minimized.

Copy link
Contributor Author

@navytux navytux commented Jan 10, 2020

@bcmills, thanks for feedback. It is true that we can avoid third-party done channel and extra goroutine if only one of the parents is cancellable. However, as you say, in general case for Merge we need to derive new done channel and link it up to P1's and P2's dones if both P1 and P2 are cancellable. The latter is the case that actually happens in practice where P1 is context of client request and P2 is context of the service itself which can be cancelled too.

For the reference: since Merge is a well defined concept, I tend to think that it makes more sense to add Merge support to context package directly instead of exposing a general mechanism for gluing arbitrary third-party contexts. At least I would consider doing it this way.

@navytux

This comment has been minimized.

Copy link
Contributor Author

@navytux navytux commented Jan 10, 2020

P.S. another point in case where context merging might be useful is #30694.

@taralx wrote in #30694 (comment) (emphasis mine):

Contexts are everywhere now, and can carry significant information, like authentication data. It is therefore useful to us to be able to set the base context to something other than context.Background().

(#20956 has a similar request, although that one is context modification per-connection instead of per-listener.)

While it is possible to do this by wrapping the handler and merging the contexts, this is error-prone and requires an additional goroutine to properly merge the Done channels.

...

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 10, 2020

I tend to think that it makes more sense to add Merge support to context package directly instead of exposing a general mechanism for gluing arbitrary third-party contexts.

I tend to agree. Want to file a separate proposal for that?

@navytux

This comment has been minimized.

Copy link
Contributor Author

@navytux navytux commented Jan 10, 2020

I tend to agree. Want to file a separate proposal for that?

Thanks. I filed it at #36503.

@navytux

This comment has been minimized.

Copy link
Contributor Author

@navytux navytux commented Feb 6, 2020

Judging by #33502 this proposal seems to have been missed. Could someone please add it to Proposals/Incoming project? Thanks.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

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