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

navytux · 2020-01-08T11:15:49Z

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

The text was updated successfully, but these errors were encountered:

bcmills · 2020-01-10T15:19:34Z

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

navytux · 2020-01-10T16:44:09Z

@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 · 2020-01-10T16:53:37Z

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 · 2020-01-10T16:55:25Z

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 · 2020-01-10T18:13:37Z

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

Thanks. I filed it at #36503.

navytux · 2020-02-06T08:23:55Z

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

golang/go#36503 golang/go#36448 They are there for 6 months already without being considered for real and so the progress is very unlikely, but still...

rsc · 2023-07-12T17:52:12Z

Now that we accepted and implemented #40221, is there anything left for this proposal?

rsc · 2023-07-12T19:54:37Z

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

navytux · 2023-07-17T05:37:51Z

No, thank you.

navytux · 2023-07-17T05:38:17Z

.

rsc · 2023-07-19T21:21:15Z

This proposal has been declined as retracted.
— rsc for the proposal review group

toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 8, 2020

toothrot added this to the Backlog milestone Jan 8, 2020

gopherbot added the Proposal label Jan 8, 2020

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

navytux mentioned this issue Jan 10, 2020

proposal: context: add Merge #36503

Closed

ianlancetaylor added this to Incoming in Proposals (old) Feb 6, 2020

navytux closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

navytux commented Jan 8, 2020 •

edited

bcmills commented Jan 10, 2020 •

edited

navytux commented Jan 10, 2020

navytux commented Jan 10, 2020 •

edited

bcmills commented Jan 10, 2020

navytux commented Jan 10, 2020

navytux commented Feb 6, 2020

rsc commented Jul 12, 2023

rsc commented Jul 12, 2023

navytux commented Jul 17, 2023

navytux commented Jul 17, 2023

rsc commented Jul 19, 2023

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

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

Comments

navytux commented Jan 8, 2020 • edited

Merging contexts

func Merge

bcmills commented Jan 10, 2020 • edited

navytux commented Jan 10, 2020

navytux commented Jan 10, 2020 • edited

bcmills commented Jan 10, 2020

navytux commented Jan 10, 2020

navytux commented Feb 6, 2020

rsc commented Jul 12, 2023

rsc commented Jul 12, 2023

navytux commented Jul 17, 2023

navytux commented Jul 17, 2023

rsc commented Jul 19, 2023

navytux commented Jan 8, 2020 •

edited

bcmills commented Jan 10, 2020 •

edited

navytux commented Jan 10, 2020 •

edited