Closed gobwas closed 4 years ago
/cc @Sajmani
I'm not convinced we want to expose the detail of the exact current Cancel signature. In general waiting for cancellation requires a goroutine and that can be heavy (not just in this one case but in other uses too). We might want to think about that more general problem than this one short-circuit.
/cc @bcmills
Bryan has brought this up in the past, that we should make it possible for custom context implementations to avoid creating goroutines like the built-in implementation does.
In this example, it seems like the worker ID could be carried as a context value without a custom implementation. This would get you the efficiency you want at the cost of a runtime type assertion to extract the worker ID.
We know that there's no way to avoid new goroutines. Maybe that's important, maybe it's not. It's honestly unclear. Maybe things are fine as they are. The specific proposal here is probably not acceptable: we don't want to commit to exposing that specific API.
@Sajmani seems like not only runtime type assertion, but also additional allocation to fill up interface{}
header field with pointer to the, say, int
ID?
There are some allocations involved in adding context values, yes. If that's causing significant overhead in your programs, could you please provide a profile? Then we can understand whether optimizations are necessary.
On Sun, Dec 23, 2018 at 8:12 AM Sergey Kamardin notifications@github.com wrote:
@Sajmani https://github.com/Sajmani seems like not only runtime type assertion, but also additional allocation to fill up interface{} header field with pointer to the, say, int ID?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/28728#issuecomment-449635597, or mute the thread https://github.com/notifications/unsubscribe-auth/AJSK3Sl1OgtOoXPjY86IQC-peii7PsH1ks5u74EkgaJpZM4YYsno .
ping @gobwas, do you have any data to answer @Sajmani's question from Dec 23?
Hello @rsc @Sajmani,
Sorry for silence.
It causing an overhead in general. Not any real world significant impact example yet. My point was to get rid of such overhead by doing a little in context’s API.
In general new API has a very high cost in constraining future implementations, what new users have to learn and so on. We don't add new API speculatively for overhead without real-world significant impact. There needs to be a real benefit to outweigh this real cost. Let's close this until we have a more pressing need.
Okay, not a problem. By the way this may be implemented as an optional interface with just a type assertion, without adding cost of learning but adding flexibility of use.
Third party context implementations suffer from performance degradation when being used alongside the standard library implementation, as every call to context.WithCancel
, context.WithTimeout
or context.WithDeadline
results in invocation of a goroutine.
Above, the proposed solution is to expose Canceler
and Tracker
interfaces in the context package. It was declined because of the need to expose new interfaces, which are not sure to be needed, and to constrain the current context standard library implementation.
I propose a compromise, in which, the current standard library implementation will use an unexported interfaces that contain exported functions. This solves the problem and addresses its disadvantages. I also propose a slightly different interface than the one in #28728. See draft implementation in https://github.com/posener/go/commit/2641c4b:
// canceler is an optional interface for a context implementation for propagating
// cancellation. If a context implementation want to manage context
// cancellation it needs to cancel all its children when cancelled.
// This interface provides a way for a child to register itself in a parent context.
// When a context implements this interface and is cancelled, it promises to
// cancel all the registered contexts which have yet to unregister.
type canceler interface {
// Register a new cancelable child for a context.
Register(cancelable)
// Unregister existing cancelable child.
Unregister(cancelable)
}
The cancelable interface is a modified version of the current canceler
interface. I found it awkward that the cancel method has the removeFromParent
argument, as this responsibility should not be on the caller. I renamed the interface since whoever implements it enables being canceled and not canceling other contexts. In all the places it is used in the code it makes more sense now (at least to me).
// cancelable is an optional interface for a context implementation that can be
// canceled directly.
type cancelable interface {
Cancel(err error)
Done() <-chan struct{}
}
// child is an optional interface for a context implementation in which it returns its
// parent context. It is useful for context to expose this interface to prevent
// an extra goroutine when being called with a cancellable context conversion.
type child interface {
// Return the parent context of a context.
Parent() Context
}
This change is fully backward compatible.
See draft implementation in https://github.com/posener/go/commit/2641c4b.
Since this proposal highly influences the design of the current context implementation, here is a draft implementation which shows the usage of these interfaces in the standard library.
A nice thing that came up from this design is that now cancelCtx
implements the cancelable
and canceler
interface, and valueCtx
implements the child
interface. The implementation of parentCancelCtx
is much clearer now (it was renamed to lookupCanceler
):
func lookupCanceler(parent Context) canceler {
for {
switch c := parent.(type) {
case canceler:
return c
case child:
parent = c.Parent()
default:
return &cancelerCtx{Context: parent}
}
}
}
From the code above, the cancelerCtx
is a wrapper around Context
that adds implementation of the canceler
interface, by invoking the a goroutine in the Register
function (Replaces the invocation in propagateCancel):
type cancelerCtx struct {
Context
}
func (c *cancelerCtx) Register(child cancelable) {
go func() {
select {
case <-c.Done():
child.Cancel(c.Err())
case <-child.Done():
}
}()
}
func (r *cancelerCtx) Unregister(child cancelable) {}
The valueCtx
implements the child
interface:
func (c *valueCtx) Parent() Context {
return c.Context
}
And the cancelCtx
implements the canceler
and cancelable
interfaces (very simple refactor of the existing logic).
@rsc can you please revise according to https://github.com/golang/go/issues/28728#issuecomment-497952715
Reopening. Note that we do not reread closed issues, so if we miss the GitHub mail, we never see them. Having reopened, this will appear in the proposal milestone searches again.
How this proposal will be making further progress from this point on?
Also: Can a specific release be assigned before proposal is marked Proposal-Accepted
?
Thank you.
The proposal review team needs to take a serious look at the suggested API above and see if it makes sense.
It would help a great deal if other people looked at it and commented.
Accepting a proposal just means that it's OK for someone to do the work. It doesn't mean that someone will actually do the work. Go is an open source project, and we can't control what contributors do and when. We assign release numbers to bug reports to try to make sure that bugs are addressed for specific releases. We use the release blocker label on some critical issues to say that we will not make a release until the issue is fixed in some way. We do not normally assign release numbers to new feature work, which is what this would be, and when we do assign release numbers they are aspirational rather than prescriptive.
@ianlancetaylor , I'm not sure why this proposal is labeled with WaitingForInfo. I would also like to change the name of the issue to "Proposal: context: Enable 1st class citizenship of 3rd party implementations" Can it make it to the decision forum? I am willing to own it if it will be accepted - I already proposed a draft implementation in posener/go@2641c4b. Thanks
I updated the title and removed the WaitingForInfo label.
CC @Sajmani
@ianlancetaylor can I help in anything to promote this?
@posener Go release dashboard - Pending proposals (summary is on the top.)
To summarize the discussion so far, this issue concerns the implementation of context.WithCancel(parent) (or context.WithDeadline, but we can focus on WithCancel).
WithCancel has signature:
func WithCancel(parent Context) (ctx Context, cancel CancelFunc)
It creates a new context ctx and cancel func and then wires them to the parent in two ways:
Right now if the parent is a known implementation from inside the context package, these steps are done by manipulating data structures, specifically p.children. But otherwise for a general parent implementation, WithCancel starts a goroutine (in propagateCancel). That goroutine waits on both parent.Done and child.Done and implements either the second half of step 1 or step 2, depending on which happens.
The problem addressed by this issue is the creation of one goroutine per WithCancel call, which lasts until the created child is canceled. The goroutine is needed today because we need to watch for the parent to be done. The child is a known implementation; the parent is not.
One thing we could do with no API changes (pointed out by @bradfitz) is to redo the internals of context to have only one goroutine per parent with an unknown implementation, making that goroutine in charge of all the parent's children. For the case where you have one parent context with many direct children, this would reduce from many goroutines to one. Even if a parent had grandchildren, as long as they were created with standard contexts you'd still end up with one goroutine.
That is, we can do one goroutine per non-standard context implementation instead of one goroutine per operation on a non-standard context implementation.
For the specific case in the top comment by @gobwas, the program today uses one background goroutine per task; the change suggested by @bradfitz would change it to just one goroutine, shared by all the tasks.
@posener and @av86743, you've both commented but I think not shown a concrete example problem motivating your interest. Would the change mentioned above (many goroutines turning into one goroutine) fix the problems you are seeing as well?
It seems like a reasonable path forward would be to improve the internals of context without any public API changes (not even unexported optional interfaces) - that will help everyone without any client code updates - and then see what problems still remain in real-world programs.
What do people think?
Regarding concrete problems, Cockroach Labs has reported the goroutine-per-context issue in an interview with their eng team in April. According to my notes, they'd like to use a custom Context implementation to record better diagnostics (e.g., why was this context canceled?), but the overhead for custom implementations is prohibitive. They also claim that the linked list implementation is slow; a custom implementation would allow them to optimize this.
I have an alternative proposal for addressing this issue, but it does require introducing an optional interface. The idea is to support efficient cancellation only through the standard Context implementations, but allow custom Context implementations to indicate "skip me when looking for a standard Context that you can cancel":
This allows custom implementations do optimize the representation of values and record custom info, e.g., by defining MyWithCancel(ctx, myReason) as context.WithCancel + a custom wrapper for myReason. The wrapper defines Parent, which allows cancelation to hop over it to find the standard context created by context.WithCancel.
Sadly, I don't have a specific use case. I heard about CockroachDB issue, and came up with fcontext.
I don't understand what is the harm of exposing unexported interfaces. If anyone implements them they will improve their performance, and if in the future we decide to drop these interfaces, it won't break anything. The solution of having only 1 goroutine sounds good, and maybe we should actually have them both - such that context implementations which don't implement the unexported interfaces will have better performance.
@Sajmani , what you propose is similar to this proposed in change https://github.com/posener/go/commit/2641c4b , but this change also solves the problem of removing a child from a parent when it is canceled.
@posener
... came up with fcontext.
fcontext
link 404-s, I'm afraid.
fcontext
link 404-s, I'm afraid.
Sorry, fixed!
We are wary of unexported interfaces because we used to use them more frequently, and we've discovered from experience that they can introduce subtle problems as code evolves. For one example, see #32306. This doesn't mean that we can't use an unexported interface; it's just that we would rather look for another approach if we can find one.
@Sajmani If I understand your suggestion correctly, it addresses the issue of people who want an optimized list of values attached to a context, but I'm not sure it addresses the original post on this issue with a WorkerContext
. For WorkerContext
, that context is itself the parent.
@ianlancetaylor Right, my proposal would require WorkerContext to have a parent Context. This is most easily done through embedding: type WorkerContext struct { Context // embedded parent Context provides Done/Deadline/Err/Value ID uint } func (wc *WorkerContext) Parent() Context { return wc.Context }
WorkerContext provides the ID, and the embedded Context provides cancelation, deadlines, etc. It's perfectly fine to embed context.Background() for a top-level WorkerContext; in this case, that context has no deadline and cannot be canceled.
We know that we could add new API to eliminate the goroutines entirely. What we want to understand is whether the internal change - with no new API - would eliminate enough goroutines not to matter anymore. We need to find out whether that is the case in real problems people have.
Change https://golang.org/cl/196521 mentions this issue: context: use fewer goroutines in WithCancel/WithTimeout
In WithCancel/WithDeadline, the context package wants to map parent back to the innermost "cancelable" context - a *cancelCtx - and then attach the child. Yesterday we were talking about how to make the package allocate fewer goroutines when that mapping fails, because parent is not a known implementation.
But what if we make the mapping succeed even when parent is an unknown implementation? Sameer is trying to get at that with the optional Parent method, but again we don't really want to add new methods, even optional ones. New API, even optional new API, complicates usage for all users, and if we can avoid that, we usually prefer to avoid it.
It occurred to me that you get almost all the way to the *cancelCtx by calling parent.Done. That is, for a not-yet-canceled context, Done returns a channel that uniquely identifies the underlying *cancelCtx. If we simply maintain a map from done channel to *cancelCtx, we have everything we need, even for a custom implementation, without any new API. Call parent.Done, look up the channel in our map, edit the associated *cancelCtx.
I sent CL 196521 doing exactly this. For any custom context implementation that passes along a done channel from an implementation created by the context package, there's no longer any goroutine.
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.
We could optimize further by applying the suggestion from yesterday, allocating only one goroutine per unknown done channel instead of one per WithCancel/WithDeadline call using a context with an unknown done channel. But since at the moment I expect there are approximately zero context with unknown done channels floating around, we may not need to do that at all.
CL 196521 has no new API, so I think it can be submitted independent of the outcome in this issue. But after submitting it, this issue may no longer be an issue.
Especially helpful at this point would be real-world examples of problems that CL 196521 does not solve; in particular, real-world examples of context implementations that allocate their own done channels.
I love the idea of mapping done channels back to parent contexts. I think it could work very well. Nice observation!
On Thu, Sep 19, 2019 at 4:11 PM Russ Cox notifications@github.com wrote:
In WithCancel/WithDeadline, the context package wants to map parent back to the innermost "cancelable" context - a *cancelCtx - and then attach the child. Yesterday we were talking about how to make the package allocate fewer goroutines when that mapping fails, because parent is not a known implementation.
But what if we make the mapping succeed even when parent is an unknown implementation? Sameer is trying to get at that with the optional Parent method, but again we don't really want to add new methods, even optional ones. New API, even optional new API, complicates usage for all users, and if we can avoid that, we usually prefer to avoid it.
It occurred to me that you get almost all the way to the cancelCtx by calling parent.Done. That is, for a not-yet-canceled context, Done returns a channel that uniquely identifies the underlying cancelCtx. If we simply maintain a map from done channel to cancelCtx, we have everything we need, even for a custom implementation, without any new API. Call parent.Done, look up the channel in our map, edit the associated cancelCtx.
I sent CL 196521 https://golang.org/cl/196521 doing exactly this. For any custom context implementation that passes along a done channel from an implementation created by the context package, there's no longer any goroutine.
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.
We could optimize further by applying the suggestion from yesterday, allocating only one goroutine per unknown done channel instead of one per WithCancel/WithDeadline call using a context with an unknown done channel. But since at the moment I expect there are approximately zero context with unknown done channels floating around, we may not need to do that at all.
CL 196521 has no new API, so I think it can be submitted independent of the outcome in this issue. But after submitting it, this issue may no longer be an issue.
Especially helpful at this point would be real-world examples of problems that CL 196521 does not solve; in particular, real-world examples of context implementations that allocate their own done channels.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/28728?email_source=notifications&email_token=ACKIVXPPKQTE2PIUQPQMJZLQKPMF7A5CNFSM4GDCZHUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7EVXJQ#issuecomment-533289894, or mute the thread https://github.com/notifications/unsubscribe-auth/ACKIVXJSIA22IJHMOLE3IG3QKPMF7ANCNFSM4GDCZHUA .
CL 196521 looks likely to land soon (to be in Go 1.14). Assuming it does, are there any real-world examples of code that would still allocate goroutines in WithCancel etc?
Hi @rsc,
Agree with @Sajmani – clever observation.
Am I understand it right, that for the example from the top comment I still need to embed some *cancelCtx
for my WorkerContext to prevent additional goroutine spawn? I mean:
type WorkerContext struct {
ID int
context.Context
cancel context.CancelFunc
}
func NewWorkerContext(id int) *WorkerContext {
ctx, cancel := context.WithCancel(context.Background())
return &WorkerContext{
ID: id,
Context: ctx,
cancel: cancel,
}
}
With the last implementation by rsc, as long as your custom context
implementation Value
method will eventually returns its parent context
Value
invocation result, no extra goroutine will be used.
On Wed, Sep 25, 2019 at 10:53 PM Sergey Kamardin notifications@github.com wrote:
Hi @rsc https://github.com/rsc,
Agree with @Sajmani https://github.com/Sajmani – clever observation.
Am I understand it right, that for the example from the top comment I still need to embed some *cancelCtx for my WorkerContext to prevent additional goroutine spawn? I mean:
type WorkerContext struct {
ID int context.Context cancel context.CancelFunc
}
func NewWorkerContext(id int) *WorkerContext {
ctx, cancel := context.WithCancel(context.Background()) return &WorkerContext{ ID: id, Context: ctx, cancel: cancel, }
}
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/28728?email_source=notifications&email_token=AAHAN7UUZ4FEOYVC2BNCJNTQLO6TLA5CNFSM4GDCZHUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7TECLY#issuecomment-535183663, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHAN7UPSPUX2SLBREIGHHLQLO6TLANCNFSM4GDCZHUA .
@posener, looks like not only Value() but Done() too.
Every context that wraps another should pass along calls to Value with unknown keys to the wrapped context. That should not be a problem.
If you want to make a custom context with a custom done channel (that is, a custom result from Done) and then use that context with WithTimeout etc, then yes, that will still incur a goroutine. Do you have a reason to do that? All the arguments for custom context implementations that I have seen are about more efficient Value methods for large sets of key-value pairs, not a custom done channel.
What would be proper place to document examples of custom context patterns?
If there is a very compelling reason to make a custom context, once that we want all users to be aware of, then we could add an example. I am honestly unaware of such a reason. The only time I've heard of custom contexts is when people want to add lots of key-value pairs, and the fact that the current system doesn't scale for that is more a bug than anything else.
I have never heard of wanting a context implementation for providing a custom done channel. And even if you did have a reason, you could still use:
ctx, cancel := context.WithCancel(context.Background())
ch = ctx.Done()
to allocate the channel in a way that would avoid the goroutine.
@rsc not 100% sure this applies here, but how about allowing a custom cancel-able context with a custom error, which I've ran into multiple times.
Simply exporting the canceler
interface and canceler.cancel
func would make custom implementations much easier and efficient without actually effecting anything in the stdlib.
@OneOfOne, if you want a custom cancelable context with a custom error, you can still use context.WithCancel
internally within its implementation. (You can forward the Done
and Value
methods and still substitute your own Err
method.)
If you want to make a custom context with a custom done channel (that is, a custom result from Done) and then use that context with WithTimeout etc, then yes, that will still incur a goroutine. Do you have a reason to do that? All the arguments for custom context implementations that I have seen are about more efficient Value methods for large sets of key-value pairs, not a custom done channel.
@rsc No, currently I can not see the case when custom done channel is required. That is, changes from CL are fine for the case I mentioned on top. Thanks!
@bcmills the problem with that is, a lot of APIs (gce/aws for example), will wrap your context with another one when you pass it, and if you cancel your custom context, it won't work.
@OneOfOne, as long as the custom context is well-behaved — that is, as long as it forwards the Value
methods for unknown keys and the Done
channel unless it injects its own channel — then it will work.
The example you provided cancels synchronously using gotip
:
(And, of course, if you want to optimize an upstream API that is adding a Done
channel, you can send them a PR to do that in terms of context.WithCancel
.)
Given that the CL landed as 0ad3686 and there have been no reports of real-world use cases that are not optimized by that change, this proposal now seems like a likely decline (no changes needed anymore).
Leaving open for a week for final comments.
Future work: provide a way to identify which custom context implementations still trigger goroutine creations. This might be as simple as providing a const-guarded block to print parent.String() when we create the goroutine.
I think should be good to document the new behavior. At least in release notes.
Added a relnote to the CL to remind us for the release notes. No final comments otherwise since https://github.com/golang/go/issues/28728#issuecomment-537588917.
Declined (performance issue resolved a different way)
@rsc wrote at https://github.com/golang/go/issues/28728#issuecomment-533289894:
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 https://github.com/golang/go/issues/28728#issuecomment-535118817:
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< ----
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(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
Appologize, once again, for being late here, and thanks beforehand for taking my example into account. Kirill
/cc @Sajmani
Hello there,
This proposal concerns a way to implement custom
context.Context
type, that is clever enough to cancel its children contexts during cancelation.Suppose I have few goroutines which executes some tasks periodically. I need to provide two things to the tasks – first is the goroutine ID and second is a context-similar cancelation mechanism:
Then, on the caller's side, the use of goroutines looks like this:
Looking at the context.go code, if the given
parent
context is not of*cancelCtx
type, it starts a separate goroutine to stick onparent.Done()
channel and then propagates the cancelation.The point is that for the performance sensitive applications starting of a separate goroutine could be an issue. And it could be avoided if
WorkerContext
could implement some interface to handle cancelation and track children properly. I mean something like this:Also, with that changes we could even use custom contexts as children of created with
context.WithCancel()
and others:Sergey.