proposal: context: add Merge #36503
Comments
|
Judging by #33502 this proposal seems to have been missed. Could someone please add it to |
Is not the struct-held reference to a context a smell (regardless that it is long-lived)? If your server must be cancellable isn't it better practice to establish a "done" channel (and select on that + ctx in main thread) for it and write to that when the server should be done? This does not incur an extra goroutine. |
|
@dweomer, as I already explained in the original proposal description there are two cancellation sources: 1) the server can be requested to be shutdown by its operator, and 2) a request can be requested to be canceled by client who issued the request. This means that any request handler that is spawned to serve a request must be canceled whenever any of "1" or "2" triggers. How does "select on done + ctx in main thread" helps here? Which context should one pass into a request handler when spawning it? Or do you propose we pass both ctx and done into all handlers and add done into every select where previously only ctx was there? If it is indeed what your are proposing, I perceive Re smell: I think it is not. Go is actually using this approach by itself in database/sql, net/http (2, 3, 4, 5, 6) and os/exec. I suggest to read Go and Dogma as well. |
|
I just reinvented this independently. The situation is that I have two long-lived things, a shared work queue which several things could be using, and the individual things, and it's conceptually possible to want to close the shared work queue and make a new one for the things to use... And then there's an operation where the work queue scans through one of the things to look for extra work. That operation should shut down if either the thing it's scanning shuts down, or the shared work queue in general gets shut down. Of course, context.Merge wouldn't quite help as one of them currently exposes a chan struct{}, not a Context. |
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...
|
I think I understand the proposal. I'm curious how often this comes up. The Merge operation is significantly more complex to explain than any existing context constructor we have. On the other hand, the example of "server has a shutdown context and each request has its own, and have to watch both" does sound pretty common. I guess I'm a little confused about why the request context wouldn't already have the server context as a parent to begin with. I'm also wondering whether Merge should take a ...context.Context instead of hard-coding two (think io.MultiReader, io.MultiWriter). |
|
I do like the MultiReader/MultiWriter parallel; that seems like it's closer to the intent. In our case, we have a disk-intensive workload that wants to be mediated, and we might have multiple network servers running independently, all of which might want to do some of that kind of work. So we have a worker that sits around waiting for requests, which come from those servers. And then we want to queue up background scans for "work that got skipped while we were too busy but we wanted to get back to it". The background scan of any given individual network server's workload is coming in parented by the network server, but now it also wants to abort if the worker decides it's closing. But the worker's not really contingent on the network server, and in some cases could be stopped or restarted without changing the network servers. It's sort of messy, and I'm not totally convinced that this design is right. I think it may only actually matter during tests, because otherwise we wouldn't normally be running multiple network servers like this at once in a single process, or even on a single machine. |
|
@seebs, if the background work is continuing after the network handler returns, it's generally not appropriate to hold on to arbitrary values from the handler's |
|
... I think I mangled my description. That's true, but it doesn't happen in this case. Things initiated from the network requests don't keep the network request context if any of their work has to happen outside of that context. They drop something in a queue and wander off. The only thing that has a weird hybrid context is the "background scanning", because the background scanning associated with a given network server should stop if that server wants to shut down, but it should also stop if the entire worker queue wants to shut down even when the network server is running. But the background scanning isn't triggered by network requests, it's something the network server sets up when it starts. It's just that it's contingent on both that server and the shared background queue which is independent from all the servers. |
|
@rsc, thanks for feedback. Yes, as you say, the need for merge should be very pretty common - practically in almost all client-server cases on both client and server sides.
For networked case - when client and server interoperate via some connection where messages go serialized - it is relatively easy to derive handler context from base context of server and manually merge it with context of request:
Here merging can happen manually because client request arrives to server in serialized form. In other cases - where requests are not serialized/deserialized - the merge is needed for real, for example:
Since, even though they are found in practice, "1" and "2" might be viewed as a bit artificial, lets consider "3" which happens in practice all the time: Consider any client method for e.g. RPC call - it usually looks like this:
Now, since there is no Merge, everyone is implementing this functionality by hand with either extra goroutine, or by doing something like , keeping registry of issued requests with their cancel in link / stream data structures, and explicitly invoking all those cancels when link / stream goes down. Here is e.g. how gRPC implements it:
And even though such explicit gluing is possible to implement by users, people get tired of it and start to use "extra goroutine" approach at some point:
In other words the logic and complexity that Merge might be doing internally, well and for everyone, without Merge is scattered to every user and is intermixed with the rest of application-level logic. On my side I would need the Merge in e.g. on client,
and on server where context of spawned handlers is controlled by messages from another server which can tell the first server to stop being operational (it can be as well later told by similar message from second server to restart providing operational service): https://lab.nexedi.com/kirr/neo/blob/85658a2c/go/neo/storage.go#L52-56 and in many other places... I often see simplicity as complexity put under control and wrapped into simple interfaces. On "3" I think the following analogies are appropriate: Without Merge context package is like
In other words Merge is a fundamental context operation. Yes, Merge requires willingness from Go team to take that complexity and absorb it inside under Go API. Given that we often see reluctance to do so in other cases, I, sadly, realize that it is very unlikely to happen. On the other hand there is still a tiny bit of hope on my side, so I would be glad to be actually wrong on this... Kirill P.S. I tend to agree about converting Merge to accept (parentv ...context.Context) instead of (parent1, parent2 context.Context). P.P.S. merging was also discussed a bit in #30694 where @taralx wrote: "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." |
|
Within Google's codebase, where the From https://github.com/golang/go/wiki/CodeReviewComments#contexts:
This rule means that at any point in the call stack, there should be exactly one applicable While merging context cancellation signals is straightforward, merging context values is not. Contexts can contain trace IDs and other information; which value would we pick when merging two contexts? I also don't see how to implement this efficiently without runtime magic, since it seems like we'd need to spawn a goroutine to wait on each parent context. Perhaps I'm missing something. |
|
For values, I don't think runtime magic is needed to avoid goroutines, but we would at least need some (subtle) global lock-ordering for the cancellation locks, since we could no longer rely on the cancellation graph being tree-structured. It would at least be subtle to implement and test, and might carry some run-time overhead. |
|
Context combines two somewhat-separable concerns: cancelation (via the Deadline, Done, and Err methods) and values. The proposed Merge function combines these concerns again, defining how cancelation and values are merged. But the example use case only relies on cancelation, not values: https://godoc.org/lab.nexedi.com/kirr/go123/xcontext#hdr-Merging_contexts I would feel more comfortable with this proposal if we separated these concerns by providing two functions, one for merging two cancelation signals, another for merging two sets of values. The latter came up in a 2017 discussion on detached contexts: #19643 (comment) For the former, we'd want something like: which would arrange for ctx.Done to be closed when cancelCtx.Done is closed and ctx.Err to be set from cancelCtx.Err, if it's not set already. The returned ctx would have the earlier Deadline of ctx and cancelCtx. We can bikeshed the name of |
|
@navytux, what do you think about Sameer's suggestion to split the two operations of WithContextCancellation and WithContextValues (with better names, probably)? |
|
@Sajmani, @rsc, everyone, thanks for feedback. First of all I apologize for the delay with replying as I'm overbusy this days and it is hard to find time to properly do. This issue was filed 7 months ago when things were very different on my side. Anyway, I quickly looked into what @Sajmani referenced in #36503 (comment), and to what other says; my reply is below: Indeed Context combines two things in one interface: cancellation and values. Those things, however, are orthogonal. While merging cancellation is straightforward, merging values is not so - in general merging values requires merging strategy to see how to combine values from multiple sources. And in general case merging strategy is custom and application dependent. My initial proposal uses simple merging strategy with values from parent1 taking precedence over values from parent2. It is simple merging strategy that I've came up with while trying to make Merge work universally. However the values part of my proposal, as others have noted, is indeed the weakest, as that merging strategy is not always appropriate. Looking into what @Sajmani has said in #19643 (comment) and #19643 (comment), and with the idea to separate cancellation and values concerns, I propose to split // CancelCtx carries deadline and cancellation signal across API boundaries.
type CancelCtx interface {
Deadline() (deadline time.Time, ok bool)
Done() <-chan struct{}
Err() error
}
// CancelFunc activates CancelCtx telling an operation to abandon its work.
type CancelFunc func()
// Values carries set of key->value pairs across API boundaries.
type Values interface {
Value(key interface{}) interface{}
}
// Context carries deadline, cancellation signal, and other values across API boundaries.
type Context interface {
CancelCtx
Values
}
// ... (unchanged)
func WithCancel (parent Context) (ctx Context, cancel)
func WithDeadline (parent Context, d time.Time) (ctx Context, cancel)
func WithTimeout (parent Context, dt time.Duration) (ctx Context, cancel)
func WithValue (parent Context, key,val interface{}) Context
// MergeCancel merges cancellation from parent and set of cancel contexts.
//
// It returns copy of parent with new Done channel that is closed whenever
//
// - parent.Done is closed, or
// - any of CancelCtx from cancelv is canceled, or
// - cancel called
//
// whichever happens first.
//
// Returned context has Deadline as earlies of parent and any of cancels.
// Returned context inherits values from parent only.
func MergeCancel(parent Context, cancelv ...CancelCtx) (ctx Context, cancel CancelFunc)
// WithNewValues returns a Context with a fresh set of Values.
//
// It returns a Context that satisfies Value calls using vs.Value instead of parent.Value.
// If vs is nil, the returned Context has no values.
//
// Returned context inherits deadline and cancellation only from parent.
//
// Note: WithNewValues can be used to extract "only-cancellation" and
// "only-values" parts of a Context via
//
// ctxNoValues := WithNewValues(ctx, nil) // only cancellation
// ctxNoCancel := WithNewValues(Background(), ctx) // only values
func WithNewValues(parent Context, vs Values) Context
For the reference, here is how originally-proposed // Merge shows how to implement Merge from https://github.com/golang/go/issues/36503
// in terms of MergeCancel and WithNewValues.
func Merge(parent1, parent2 Context) (Context, cancel) {
ctx, cancel := MergeCancel(parent1, parent2)
v12 := &vMerge{[]Values{parent1, parent2}}
ctx = WithNewValues(ctx, v12)
return ctx, cancel
}
// vMerge implements simple merging strategy: values from vv[i] are taking
// precedence over values from vv[j] for i>j.
type vMerge struct {
vv []Values
}
func (m *vMerge) Value(key interface{}) interface{} {
for _, v := range m.vv {
val := v.Value(key)
if val != nil {
return val
}
}
return nil
}Regarding implementation: it was already linked-to in my original message, but, as people still raise concerns on whether "avoid extra-goroutine" property is possible, and on lock ordering, here it is once again how libgolang implements cancellation merging without extra goroutine and without any complex lock ordering: https://lab.nexedi.com/nexedi/pygolang/blob/0e3da017/golang/context.h Maybe I'm missing something, and of course it will have to be adapted to Kirill /cc @zombiezen, @jba, @ianlancetaylor, @rogpeppe for #19643 |
|
Thanks for the reply. We're probably not going to split the Context interface as a whole at this point. The value set merge can be done entirely outside the context package without any inefficiency. And as @neild points out, it's the part that is the most problematic. The cancellation merge needs to be inside context, at least with the current API, or else you'd have to spend a goroutine on each merged context. (And we probably don't want to expose the API that would be required to avoid that.) So maybe we should focus only on the cancellation merge and ignore the value merge entirely. It still doesn't seem like we've converged on the right new API to add, though. @bradfitz points out that not just the cancellation but also the timeouts get merged, right? It does seem like the signature is Or maybe the op to expose is being able to cancel one context when another becomes done, like: (with better names). ? It seems like we're not yet at an obviously right answer. |
|
@rsc, thanks for feedback. I think I need to clarify my previous message:
Regarding What do you think? Does my feedback clarify anything? Kirill |
|
FWIW, @Sajmani's comment from 2017 #19643 (comment) is out of date. WithNewValues can be implemented efficiently outside the context package, after changes we made recently. Re: MergeCancel(parent Context, cancelCtx ...Context) being "worse overall because it looses generality", what generality does it lose? No one has any values of type CancelCtx today, so there is nothing to generalize. Even if we added the CancelCtx type, wouldn't every instance be a Context anyway? Certainly the only ones we can handle efficiently would be contexts. It does sound like we're converging on Does anyone object to those semantics? Maybe it should be MergeDone? Some better name? |
|
I have a use case with context as the carrier for the cancellation signal. It allows me to achieve simplicity by decoupling my component from application lifecycle management. Here is an example package that describes the approach in more detail: |
|
@Sajmani, @rsc, everyone, thanks for feedback. The main criticisms of hereby proposal are a) that there is no real compelling use case for Let's go through those: The need for
|
@navytux While I like the MergeCancel idea, it should be noted actual implementation can be much simpler - at cost of adding 1 extra goroutine per method call (same 1 extra goroutine which is currently created anyway by 3rd-party MergeCancel implementations): struct Service {
...
stopped chan struct{}
}
func (srv *Service) Stop() {
close(srv.stopped)
}
func (srv *Service) DoSomething(ctx context.Context) (err error) {
ctx, cancel := context.WithCancel(ctx)
defer cancel()
go func() {
select {
case <-srv.stopped:
cancel()
case <-ctx.Done():
}
}()
return thirdparty.RunJob(ctx)
} |
|
Will move back to active instead of likely decline. |
|
Note that #57928 may be a better or equally good answer, since it would let people implement Merge efficiently outside the standard library. |
|
Thanks, Russ. Regarding your note I'd like to point out that "let people implement X outside the project" might be not universally good criteria. For example from this point of view we could also say that "with callbacks there is no need for channels and goroutines to be built-in, since with callbacks those primitives could be implemented outside". I might be missing something but in my view what makes sense to do is to provide carefully selected and carefully thought-out reasonably small set of high-level primitives instead of lower-level ones for "doing anything outside". In #36503 (comment) I already explained that MergeCancel is fundamental context operation paralleling it to merges in git, Kirill P.S. @powerman, thanks. I was implicitly assuming we want to avoid that extra goroutine cost, but you are right it would be better to state that explicitly and to show plain solution as well. |
|
@navytux, thanks for the detailed scenario for helping to think about this proposal. When people have asked me about how to handle these situations in the past I have usually encouraged them to figure out how to get the two contexts to have a common parent context rather than try to figure out how to merge unrelated contexts. In your example it doesn't appear that is possible at first glance because the context passed to Isn't there always a higher scope in the application that manages the lifetime of the For example: Granted, the higher scope has to orchestrate more this way, but I haven't found that difficult to manage in the code I've worked on. I am curious if there is a fundamental reason why it shouldn't be done this way? |
|
Abstract examples are difficult to work with. What is a Service? Why does it stop? Should stopping a service (possibly optionally) block until outstanding requests have been canceled? Should stopping a service (possibly optionally) let outstanding requests finish before stopping? If either of these features are desired, then context cancellation isn't sufficient; the service needs to be aware of the lifetimes of outstanding requests, which contexts don't provide. I'd expect any network server, such as an HTTP or RPC server, to support graceful shutdown where you stop accepting new requests but let existing ones run to completion. But perhaps a Service in this example is something else. I still feel that the motivating case for MergeCancel is unconvincing. However, even granting a compelling use case, MergeCancel confuses the context model by separating cancellation and value propagation, cannot be implemented efficiently in the general case, and does not address common desires such as bounding a network operation or condition variable wait by a context's lifetime. |
|
This proposal has been added to the active column of the proposals project |
@neild Please don't take this reply as one "for MergeCancel" and "against context.After" - I just like to clarify this use case for you. For trivial fast RPC calls - you right, it's usually preferable to complete current requests on graceful shutdown instead of interrupting them. But some RPC can be slow and also RPC can be streaming (i.e. never ending) - in these cases cancelling them using context is really natural way to go. Also, once again, it's worth to remind about less common but valid use case when we need to cancel some group of requests but not all of them - i.e. not a graceful shutdown case. It may be requests of some user account which was blocked by admin or runs out of money, may be requests from some external service/client whose certificate has just expired, may be requests related to some deprecated API which deprecation time happens to begin right now, cancelling a group of jobs, etc. And, yeah, here I'm still talking about slow/long-running/streaming type of requests. |
|
Waiting on #57928, but assuming we do find a good answer there, it will probably make sense to let that one go out and get used before we return to whether Merge needs to be in the standard library. |
|
Having accepted #57928, it seems like we can decline this and let people build it as needed with AfterFunc. Do I have that right? |
|
I believe that's right; AfterFunc should make it simple to efficiently implement MergeCancel. |
|
Apologies folks, if you got this via the email, I got stung by github's "oops you hit control-enter and now it's sent" misfeature and sent this too early, I have edited since to clean up some of the clumsy wording. Seems better to leave it on hold to me. Merge is a very useful building block and even if it's built on AfterFunc, I think it still merits inclusion, but I don't think we'll know until AfterFunc has been out in the wild for a bit. The shutdown propagation thing is the most valuable use case I've encountered. I'm aware that there are other preferences for how to implement that, but nothing to me has been cleaner or easier to explain to other engineers than merged contexts. Graceful shutdowns in complex systems are hard enough that I rarely see them even attempted in practice, much less done well. I think there are many ways of composing context cancellations that are underserved by the standard library, not just Merge, Merge is a useful primitive building block that I think can help unlock this space a bit (along with relaxing the guidance on context retention, which I think is restrictive, in favour of something more nuanced), and if it's simple enough to implement on top of AfterFunc, I can't help but wonder if that's actually reason to include it, not a reason not to, especially given there's a lot of interest here, a lot of examples, and a lot of +1s. Part of the problem with some of the examples given in this thread, all of which seem somewhat familiar to me in some ways and very alien in others, is that it's really hard to give a good, narrow example of exactly how we have seen a need for something like this. It's usually in the context of a much bigger problem, with constraints and nuances that are very hard to relay in the bounds of a small example. It's really one of those things where you have to feel the fiber of the fabric of it between your fingers to know. This can lead us to a lot of strawmanning of examples and suggestions that inappropriate alternatives are the Absolute Right Way, even though we don't really know enough about somebody's problem, and whether this would in fact be a good solution, to make that judgement. This feels like a case where the challenge has been to come up with a small-enough demonstration to be convincing. Maybe that's a sign that in this case, "is there a concrete use case? is the wrong question, and a better one might be "do enough folks see value in the potential unlocked by something composable and is the implementation simple enough to just let the flowers bloom?" Just my 2c, but I think it's best to leave this on hold for a while, see how things shake out after AfterFunc has had a chance to percolate, then re-evaluate with a less hard-edged approach to demanding perfectly sculpted examples. If it is indeed simple enough to build on top of AfterFunc, I think that's a reason to include it. |
|
We can decline this for now and then feel free to open a new proposal for Merge in a year or so if there is new evidence. |
|
Based on the discussion above, this proposal seems like a likely decline. |
|
No change in consensus, so declined. |
and others
EDIT 2023-02-05: Last try: #36503 (comment).
EDIT 2023-01-18: Updated proposal with 2 alternatives: #36503 (comment).
EDIT 2023-01-06: Updated proposal: #36503 (comment).
EDIT 2020-07-01: The proposal was amended to split cancellation and values concerns: #36503 (comment).
( This proposal is alternative to #36448. It proposes to add
context.Mergeinstead of exposing generalcontextAPI for linking-up third-party contexts into parent-children tree for efficiency )Current
contextpackage API provides primitives to derive new contexts from one parent -WithCancel,WithDeadlineandWithValue. This functionality covers many practical needs, but not merging - the case where it is neccessary to derive new context from multiple parents. While it is possible to implement merge functionality in third-party library (ex. lab.nexedi.com/kirr/go123/xcontext), with current state ofcontextpackage, such implementations are inefficient as they need to spawn extra goroutine to propagate cancellation from parents to child.To solve the inefficiency I propose to add
Mergefunctionality tocontextpackage. The other possibility would be to expose general mechanism to glue arbitrary third-party contexts into context tree. However since a)Mergeis a well-defined concept, and b) there are (currently) no other well-known cases where third-party context would need to allocate its owndonechannel (see #28728; this is the case where extra goroutine for cancel propagation needs to be currently spawned), I tend to think that it makes more sense to addMergesupport tocontextpackage directly instead of exposing a general mechanism for gluing arbitrary third-party contexts.Below is description of the proposed API and rationale:
---- 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 Merge
Merge merges 2 contexts into 1.
The result context:
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
ctxandsrv.serveCtxdone 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.Mergeimplementation 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
/cc @Sajmani, @rsc, @bcmills
The text was updated successfully, but these errors were encountered: