golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.11k stars 17.68k forks source link

proposal: context: add Merge #36503

Closed navytux closed 1 year ago

navytux commented 4 years ago

EDIT 2023-02-05: Last try: https://github.com/golang/go/issues/36503#issuecomment-1418138423. EDIT 2023-01-18: Updated proposal with 2 alternatives: https://github.com/golang/go/issues/36503#issuecomment-1396216800. EDIT 2023-01-06: Updated proposal: https://github.com/golang/go/issues/36503#issuecomment-1372860013.
EDIT 2020-07-01: The proposal was amended to split cancellation and values concerns: https://github.com/golang/go/issues/36503#issuecomment-652542943.


( This proposal is alternative to https://github.com/golang/go/issues/36448. It proposes to add context.Merge instead of exposing general context API for linking-up third-party contexts into parent-children tree for efficiency )

Current context package API provides primitives to derive new contexts from one parent - WithCancel, WithDeadline and WithValue. 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 of context package, 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 Merge functionality to context package. The other possibility would be to expose general mechanism to glue arbitrary third-party contexts into context tree. However since a) Merge is a well-defined concept, and b) there are (currently) no other well-known cases where third-party context would need to allocate its own done channel (see https://github.com/golang/go/issues/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 add Merge support to context package 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 (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:

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

/cc @Sajmani, @rsc, @bcmills

powerman commented 1 year ago

@neild This is going to be a long reply, but my point (tldr) is: there are clearly some common use cases and demand in merging contexts (and a lot of existing implementations mentioned by @Sajmani proves it), so main question "is it worth/possible to add efficient merge implementation in stdlib" and not "should we merge contexts at all".

(1) Contexts are not a good mechanism for bounding object lifetimes, which calls the motivating example into question.

Probably this is because they're bounding operation lifetimes, not object lifetimes.

There are already some motivating examples above, including really common one about cancelling gRPC handler either on gRPC request cancellation or on whole app graceful shutdown. One may say it's because of gRPC implementation's bad design, because it should use user-provided base context instead of Background. But there are other cases when handler gets input data from different sources with different cancellation - and it's unclear is all of them should/can be fixed by using "right design".

This association of operation lifetimes with function calls is a primary reason why Google's Go style guidelines forbid storing contexts within other types.

Sure, to have app's shutdown context in gRPC handler you have to store it in handler's object. Sure, there are really good reasons to avoid storing contexts within other types, but that's because in most cases this is leading to/needed for misusing contexts. But any rule has an exception, and I don't see anything wrong if this is used to deliver app's shutdown context into gRPC handlers, just because there are no other way to do this.

In addition, contexts provide no facility for ordering cleanup operations.

defer already does this good enough. Anyway, how is this related to the proposal?

These limitations make contexts a poor choice for bounding the lifetime of an object.

Yep. Probably this is because they're bounding the lifetime of operation, not object.

It sounds like some misunderstanding happens here, either to you or to me. The goal of merging context in gRPC handler is not to limit the lifetime of object which contains these handlers (and contains app's ctx inside object's field), the goal is to interrupt operation itself using two different sources of cancellation signals.

To make this a bit more clear let's imagine case when user pays for CPU time and then runs long operations: cancellation may comes both from billing and from user (who cancels current request) - and both should be able to cancel user's current operations. Neither of these bounds the lifetime of any object - it's not app shutdown, app continue to work and handle other user's operations, and even this user may continue using the app after depositing some more money - only operations of that user which was running at the moment when user's balance become zero should be cancelled.

Sure, there are ways to implement this without merging context. E.g. gRPC handler may on start create new CancelFunc from gRPC context and call some global app.RegisterShutdownHandler(cancel) to make sure this handler's context will be cancelled in both use cases. And then in defer also call app.UnregisterShutdownHandler(cancel). To me - merging context looks much more natural than this.

The motivating example for this proposal is a service object which is instructed to shut down when a context is cancelled. This is a dubious design; a long-running object of this nature should almost certainly have an explicit Close or Shutdown operation rather than relying on a context.

How exactly Close/Shutdown methods on object which contains gRPC method handlers can be used to cancel such gRPC handlers? By doing same Register/Unregister dance like above?

(2) There are no clear motivating examples for combining context values.

My example with having metrics in app's context and gRPC metadata in gRPC's context doesn't counts?

(3) Splitting cancellation and values adds confusion.

There is another point of view on this: joining both in single value has already added a LOT of confusion. Anything we're doing here unlikely adds more confusion. To me it's actually the opposite: ability to split these values and manage as we need makes it more clear and ease to use.

(4) I do not see how to implement cancellation merging efficiently.

As a more general point, I would like to see a reference implementation before any proposal here is accepted, so we can properly evaluate the amount of complexity being taken on.

Here I'm 100% with you!

(5) This proposal does not sufficiently address the functionality gap.

This proposal addresses the inefficiency of cancelling one context when another context completes, but it does not address cases such as cancelling an operation on a net.Conn. If we address the general case, then MergeCancel can be efficiently and simply implemented in user code.

This may be a valid point, but it may also result in having nothing as result because we make simple "merge contexts" issue into something too general and huge.

rsc commented 1 year ago

Talked with @Sajmani a bit yesterday about this. It seems like there's some evidence (and intuition) for MergeCancel, but not as much for MergeValues. So focusing on cancellation may make sense.

rsc commented 1 year ago

What would be most useful is a compelling use case for MergeCancel. https://github.com/golang/go/issues/36503#issuecomment-1397395923 pokes holes in some of the uses that have been suggested.

zikaeroh commented 1 year ago

What would be most useful is a compelling use case for MergeCancel. #36503 (comment) pokes holes in some of the uses that have been suggested.

Did you mean MergeValues? (Given the previous comment is saying there's evidence for MergeCancel, I assume so?)

prochac commented 1 year ago

My use case is calling cleanup/deferred close-like function that also takes context though. Something like this

If the original context was cancelled, then passing it to those would be pointless. So mostly, I use context.Background + timeout. That solves the cancellation, but not context values. Let's say, that the shutdown use OpenTracing, now I'm missing a span information.

TBH,

context.WithTimeout(ctx.OnlyValues(), 4*time.Day)

or

context.WithTimeout(context.MergeValues(context.Background(), ctx), 4*time.Day)

I don't care much. Maybe, I would vote for the less verbose here. But that would lead to context.Context interface bloat.

rsc commented 1 year ago

I was asking for compelling use cases for MergeCancel. I thought we had them, but https://github.com/golang/go/issues/36503#issuecomment-1397395923 pointed out, for example, that server shutdown should probably not be represented as a context, which was fundamental to the motivating example I've had in my head.

I am assuming this issue is only about MergeCancel. MergeValues is both easier to implement and more difficult to imagine use cases for.

zikaeroh commented 1 year ago

Thanks for clarifying; I guess my example on https://github.com/golang/go/issues/36503#issuecomment-1397505456 wasn't all that compelling, maybe. In my case, contexts are more likely to be canceled by timeouts/deadlines; either imposed by my own handlers (just so things don't go awry), or by the message handler library (as it may have its own expected deadline before telling the message broker to try again). Shutdown is another source as my application has a root context that uses the new os/signal-based cancellation, but is of course much rarer than those other sources.

I will say that it feels like this proposal could be implemented efficiently using #57928 (and I'll probably do so in my version of this function, if that proposal is accepted, as it does save a goroutine), so to me that strikes out the point of "can't be implemented efficiently".

powerman commented 1 year ago

I thought we had them, but #36503 (comment) pointed out, for example, that server shutdown should probably not be represented as a context, which was fundamental to the motivating example I've had in my head.

It's really not obvious and counter-intuitive (even for you, @rsc :smile:) that server shutdown should PROBABLY NOT be represented as a context. But let's assume @neild right about this for a minute. Can someone show me how server shutdown should be implemented in a "right way" (which is using Shutdown() method on object which contains gRPC method handlers if I get @neild idea right)? What exactly such Shutdown() method should do to interrupt already running gRPC method handlers?

I'm asking because obvious solution which doesn't involve contexts and merging cancellation looks like func (server *Server) Shutdown() { close(server.shutdown) }, but this means each and every gRPC handler method (and all code it calls) must check both gRPC-provided ctx and one more shutdown channel in each and every place they currently handle ctx.

adamluzsi commented 1 year ago

If the original context was cancelled, then passing it to those would be pointless. So mostly, I use context.Background + timeout. That solves the cancellation, but not context values. Let's say, that the shutdown use OpenTracing, now I'm missing a span information.

@prochac, would the context.Detach proposal solve your issue?

I thought that MergeCancel is more about merging multiple cancellations, for example merging the shutdown signal and request cancellation.

neild commented 1 year ago

Can someone show me how server shutdown should be implemented in a "right way"

The way net/http.Server handles shutdown and contexts seems fairly reasonable to me (aside from the need to pass the Context to handlers hidden inside the Request, since the handler interface predates contexts).

For an object which contains handler methods called by an http.Server (which seems analogous to the gRPC case you describe), I'd stop calls to those methods by shutting down the http.Server.

powerman commented 1 year ago

For an object which contains handler methods called by an http.Server (which seems analogous to the gRPC case you describe), I'd stop calls to those methods by shutting down the http.Server.

The problem with gRPC here is support for very long running operations (streaming RPC), so this (stop calling NEW methods) won't work. But *grpc.Server does have GracefulShutdown() method, which may cancel request contexts (this isn't documented, so needs some testing).

So, if I get your idea right: if we've two independent contexts A and B and want to cancel B when A is cancelled, then we should ask code which creates B to cancel it (if it support such a feature).

rsc commented 1 year ago

At the moment we appear to be waiting for a compelling use case for MergeCancel (https://github.com/golang/go/issues/36503#issuecomment-1401346029). Perhaps we should wait until we have a clearer use case to move forward.

MergeValues doesn't have a use case either but it can be done much more easily outside the standard library.

rsc commented 1 year ago

Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group

deefdragon commented 1 year ago

I still think that the points that @powerman brought up here are good enough support for MergeCancel(). Even if GracefullShutdown is the perferred method, I feel its much more difficult to track the flow if you use GracefullShutdown.

Beyond that however, I'm making a service that multiple clients connect to via web socket. When one client in a group closes/leaves, some common cleanup needs to be performed. To me, the concept of merging contexts and waiting on the single resulting context to be done just makes sense for this.

adamluzsi commented 1 year ago

I have a use case with context as the carrier for the cancellation signal. It doesn't help MergeCancel's case, but it's an example of when the context is used to signal shutdown.

It allows me to achieve simplicity by decoupling my component from application lifecycle management. For example, I have a high-level component to handle shutdown signals and background jobs, and these jobs use a context as a parent context for all sub-interactions. If the shutdown is signalled, all cleanup is taken care of as the synchronous process calls finish their execution and return.

Here is an example package that describes the approach in more detail: https://github.com/adamluzsi/frameless/tree/main/pkg/jobs

navytux commented 1 year ago

@Sajmani, @rsc, everyone, thanks for feedback. I could find the time to reply only today. I apologize for the delay.

The main criticisms of hereby proposal are

a) that there is no real compelling use case for MergeCancel, and b) that the number of places where MergeCancel could be useful is small.

Let's go through those:

The need for MergeCancel

Even though the original overview of this proposal does not say anything regarding that Service is stopped via context cancellation, such argument was used against hereby proposal with saying that it is not ok to bind a Service lifetime to context. Given that argument, I believe, it makes sense to describe everything once again from scratch:

Imagine we are implementing a Service. This service has dedicated methods to be created, started and stopped. For example:

type Service { ... }
func NewService() Service
func (srv *Service) Run()
func (srv *Service) Stop()

The Service provides operations to its users, e.g.

func (srv *Service) DoSomething(ctx context.Context) error

and we want those operations to be canceled on both

a) cancellation of ctx provided by user, and b) if the service is stopped by way of srv.Stop called from another goroutine.

So let's look how that could be implemented. Since inside DoSomething it might need to invoke arbitrary code, including code from another packages, it needs to organize a context that is cancelled on "a" or "b" and invoke that, potentially third-party code, with that context.

To organize a context that is cancelled whenever either user-provided ctx is cancelled, or whenever Service.Stop is called we could create a new context via WithCancel, and register its cancel to be invoked by Service.Stop:

struct Service {
    ...

    // Stop invokes functions registered in cancelOnStop
    stopMu       sync.Mutex
    stopped      bool
    cancelOnStop set[context.CancelFunc]
}

func (srv *Service) Stop() {
    srv.stopMu.Lock()
    defer srv.stopMu.Unlock()

    srv.stopped = true
    for cancel := range srv.cancelOnStop {
        cancel()
    }

    ...
}

func (srv *Service) DoSomething(ctx context.Context) (err error) {
    ctx, cancel := context.WithCancel(ctx)
    defer cancel()

    srv.stopMu.Lock()
    if srv.stopped {
        return ErrServiceDown
        srv.stopMu.Unlock()
    }
    srv.cancelOnStop.Add(cancel)
    srv.stopMu.Unlock()

    defer func() {
        srv.stopMu.Lock()
        defer srv.stopMu.Unlock()
        srv.cancelOnStop.Del(cancel)

        if err != nil && srv.stopped {
            err = ErrServiceDown
        }
    }()

    return thirdparty.RunJob(ctx)
}

This pattern is scattered and kind of used everywhere including in the gRPC internals I explained in detail in https://github.com/golang/go/issues/36503#issuecomment-644669682.

It is also not always implemented in such simple form, as people sometimes use dedicated registry for cancels, and sometimes they keep on other objects and retrieve created cancel func indirectly from there which makes the logic more scattered and harder to follow.

But what all this code is doing in a sense - is that it is duplicating functionality of context package by manually propagating cancellation from Stop to every spawned job. Let me say it once again:

_Every_ service implementation has to _manually_ propagate the
cancellation from its `Stop` or `Shutdown` to _every_ spawned job in
_every_ of its methods.

Even though this could be done, does the need for manual duplication of the context functionality suggest that functionality should be provided by the context package in the first place?

And because this propagation is usually scattered, searching code for it does not yield results with simple queries like Merge(Context). Most people keep on copying the pattern from one place to another preferring not to go against official guidelines not to store context in structures: it hides the problem if the context and propagation logic is stored in expanded form.

For the reference: Detach operation is significantly easier to implement and does not need to go against style guide that context should not be stored anywhere. That's why searching for it yields more results. But the interpretation of the search should be normalized to difficulty and willingness to go against existing recommendation with risking to receive "obey dogma" feedback.

For the reference 2: Go is actually storing contexts inside structures, e.g. in database/sql, net/http (2, 3, 4, 5, 6) and os/exec.

Actually in 2017 in https://github.com/golang/go/issues/22602#issuecomment-344980503 @Sajmani wrote

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.

so I guess that the restriction that context should not be kept in structs, should not be that strong now (/cc @cespare).

For the reference 3: and even if context.OnDone proposal is accepted, without adding MergeCancel to standard package context, the scheme of manual cancel propagation from Stop to operations will need to be kept in exactly the same form as it is now and everywhere because people still get a message that "storing serverCtx in struct is not ok". By the way, why it is context.OnDone instead of chan.OnDone? If we have a way to attach callback to channel operations many things become possible without any need to modify context or other packages. Is that path worth going? I mean internally OnDone might be useful, but is it a good idea to expose that as public API?

For the reference 4: for cancelling operations on net.Conn the solution, from my point of view, should be to extend all IO operations to also take ctx parameter and cancel IO on ctx cancel. This is the path e.g. xio package takes. And internally, after an IO operation was submitted, the implementation of that IO on Go side does select on IO completion or ctx cancel, and if ctx is cancelled issues cancel command via io_uring. That's how it should work. True, we could use wrappers over existing Reader and Writer that call Close via ctx.OnDone. While it somewhat works, closing the IO link on read cancel is not what read canceler really expects. And going this way also exposes the inner details of context machinery as public API. While that, once again, might work in the short term, in my view that won't be a good evolution in the longer run.


Another critic note regarding hereby proposal was that there is no proof-of-concept implementation shown.

But the proof of concept implementation is there and was pointed out originally right in the first message of hereby proposal. Here it is once again:

https://lab.nexedi.com/kirr/pygolang/blob/39dde7eb/golang/context.h https://lab.nexedi.com/kirr/pygolang/blob/39dde7eb/golang/context.cpp https://lab.nexedi.com/kirr/pygolang/blob/39dde7eb/golang/context_test.py https://lab.nexedi.com/kirr/pygolang/blob/39dde7eb/golang/libgolang.h

It is in C++, not Go, but it shows that the implementation is straightforward.


At this stage I have very little hope that this proposal will be accepted. As it looks it will likely be declined next Wednesday and closed. That's sad, but I will hopefully try to find my way.

Kirill

powerman commented 1 year ago

To organize a context that is cancelled whenever either user-provided ctx is cancelled, or whenever Service.Stop is called we could create a new context via WithCancel, and register its cancel to be invoked by Service.Stop:

@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)
}
rsc commented 1 year ago

Will move back to active instead of likely decline.

rsc commented 1 year ago

Note that #57928 may be a better or equally good answer, since it would let people implement Merge efficiently outside the standard library.

navytux commented 1 year ago

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 https://github.com/golang/go/issues/36503#issuecomment-644669682 I already explained that MergeCancel is fundamental context operation paralleling it to merges in git, select in go and φ in SSA. From this point of view adding MergeCancel makes sense to do because it makes context operations to be kind of full closure, which were previously incomplete.

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.

ChrisHines commented 1 year ago

@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 Service.DoSomething isn't related to anything inside Service.Stop. But maybe it's not as difficult as it seems.

Isn't there always a higher scope in the application that manages the lifetime of the Service and also the code that calls DoSomething? It seems possible that the higher scope can ensure that there is a base context that gets canceled when Service.Stop is called.

For example:

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    svc := NewService()
    go func() {
        svc.Run() // FWIW, I usually write Run(ctx) here, and avoid Stop methods.
    }()

    go func() {
        <-ctx.Done()
        svc.Stop()
    }()

    DoThingsWithService(ctx, svc)
}

func DoThingsWithService(ctx context.Context, svc *Service) {
    ctx, cancel := context.WithTimeout(ctx, time.Second)
    defer cancel()

    // ....

    svc.DoSomething(ctx)
}

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?

neild commented 1 year ago

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. context.OnDone/context.AfterFunc as proposed in #57928 fits neatly within the context model, lets us efficiently implement operations on third-party contexts that are inefficient today, and can be easily used to implement MergeCancel or the equivalent.

rsc commented 1 year ago

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

powerman commented 1 year ago

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.

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

rsc commented 1 year ago

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.

rsc commented 1 year ago

Having accepted #57928, it seems like we can decline this and let people build it as needed with AfterFunc. (They could build it before using a goroutine, but now AfterFunc lets them avoid the goroutine.)

Do I have that right?

neild commented 1 year ago

I believe that's right; AfterFunc should make it simple to efficiently implement MergeCancel.

shabbyrobe commented 1 year ago

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.

rsc commented 1 year ago

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.

rsc commented 1 year ago

Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group

rsc commented 1 year ago

No change in consensus, so declined. — rsc for the proposal review group