proposal: context: enable first class citizenship of third party implementations (case when custom context needs to create is own done channel)
Closed this issue · 11 comments
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
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, 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.
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.
...
I tend to think that it makes more sense to add
Mergesupport tocontextpackage 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?
I tend to agree. Want to file a separate proposal for that?
Thanks. I filed it at #36503.
Judging by #33502 this proposal seems to have been missed. Could someone please add it to Proposals/Incoming project? Thanks.
Now that we accepted and implemented #40221, is there anything left for this proposal?
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
No, thank you.
.
This proposal has been declined as retracted.
— rsc for the proposal review group