-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proposal: context: add API for performing tasks after Context is Done #19643
Comments
Within the |
Done. |
ctx, cancel := context.WithCancel(ctx)
finc := make(chan struct{})
go func() {
defer close(finc)
... ctx ...
}() My point is, you've carved out a nice API that is independent of detach semantics. Is it worth separating the two? |
@jba We have found that separating the detachment semantics is quite error-prone. It's easy to detach and then use the context in a synchronous manner, which isn't how it should be used. |
When you say "may return" I take it that you mean that
That's an odd requirement. It also doesn't make sense as written. Do you mean that methods of |
@ianlancetaylor Clarified the |
Why do we need to say that |
Because we don't want |
OK, it's not a big deal, but I sort of think we should say that instead. The current docs seem to be writing a useful guideline into the API. How about "Detach and RegisterPreserveFunc must not be called concurrently." |
Since both |
I'm fine with guidance, I just don't think we should confuse guidance with API documentation. |
Is RegisterPreserveFunc necessary? type Preserver interface {
// Preserve takes a value stored in a parent Context and returns the
// corresponding value to be stored in a Context derived from it.
Preserve(interface{}) interface{}
}
type Releaser interface {
// Release takes a value returned by Preserve. It is called after the
// detached task associated with the value completes.
Release(interface{})
}
// Detach starts a new goroutine that calls f with a new Context that derives
// values from parent.
//
// The values in the new Context are obtained by calling the Preserve
// method for each registered key. When f returns, the key's Release
// method (if any) will be called with the new value. Keys that do not
// implement Preserver are not associated with any value in the new
// Context. If Preserve for a key returns a nil value, the new Context
// will not have a value for that key.
func Detach(parent Context, f func(ctx Context)) DetachedTask |
@neild This is a subtle one. If this were purely an addition to |
My gut feeling is that I'm not keen, but I'd like to see more concrete use cases. Firstly, IgnoreCancel seems easy to do, not too controversial and The fact that one cannot iterate across keys makes Detach problematic from I haven't seen much explanation of why the detached context values need Why is it not viable to inherit all keys from the parent context but |
Yes, that is an unfortunate downside of the
True, but in practice it is rare for the keys not to be singletons. (If the keys are some dynamic value, how is that value made known to the caller? It's either stored under some other key in the
Yep, that's a good point.
Consider a context-scoped logging package. At some point you need to flush the log entries to their destinations (disk or a monitoring server or similar). The
No: the Done channel is closed as soon as the caller is no longer interested in the result, but you have no guarantee that other users of the
That is another viable approach, but it is more prone to unintended extension of It also retains a reference to the parent |
One possible alternative to That might look something like: package context
// Ranger is an optional interface allowing iteration over keys and values in a Context.
type Ranger interface {
// Range invokes the given function sequentially for every key and value stored
// in the Context, in no particular order.
//
// If the function returns false for any key, Range immediately returns nil.
//
// If some parent Context does not implement Ranger, Range returns ErrRangeIncomplete
// after calling the function for all known keys and values.
Range(func(key, value interface{}) bool) error
}
// ErrRangeIncomplete indicates that a call to Range may have omitted some keys
// due to a parent Context that does not implement the Ranger interface.
var ErrRangeIncomplete = … That leaves open the question of what |
/cc @Sajmani |
I like the detached value registry better than the I'd like to avoid having the registry be a part of the I propose to define a new
Since
(Some risk of arg-swapping confusion in The A separate package can now define how to get the
I prefer the Are there any implementation reasons why using a separate |
@Sajmani Your proposal would satisfy the goal. However, I'm not sure how much utility |
I'm quite scared of the complexity being proposed here. This feels like atexit for contexts, with all the relevant bugs. So I'm going to push back on this a bit:
Examples please? |
A few examples:
|
Why is it not enough to just implement (even outside the context package) context.WithoutDeadline, that returns a Context implementation that has no deadline but reports the same key-values as the original? |
Because the values stored in the Context are scoped to some particular call tree, and some of them (such as tracing and other per-request logging) need to flush entries to the log once the corresponding operation is finished. If you know exactly which per-context logging systems are in use, |
Context works because it is small. The current proposal roughly doubles the API surface, and I can't see that it even covers all the important cases. My point about context.WithoutDeadline is that having it would let you pass the context to things that shouldn't care the deadline expired. That seems defensible, and also easy to implement outside the context package. I admit that this does not provide any kind of "atexit" functionality to automatically schedule work when the context expires, but that's a feature. There is no well-defined point when the "main work" is done, at least not that the context implementation knows. The kinds of things listed in #19643 (comment) should be run explicitly, not magically as part of a context finishing. |
Agreed. The problem is that with the current API, there is no valid The only step which we are proposing should be done "magically" is committing the |
I'm back to not understanding. Now it sounds like you're saying that (1) you want to run some background operation like flush logs, but (2) before that you want to run a background operation like replicate to other servers, and that operation might generate logs. So now it seems like we're kind of talking about a whole job-scheduling system baked into Context. The tell here is "Detach starts a new goroutine...". In general we avoid APIs that do that. Users should be in charge of creating new goroutines, not libraries. There are exceptions, like time.AfterFunc and maybe finalizers, but those are exceptions, not the rule. I don't think the situation here is nearly strong enough. (I think there was a proposal to add that kind of thing to WaitGroup and we declined that too, but I can't find it.) |
There's not really any scheduling to it. It is, essentially, "run this function and then flush the Context logs". It just happens that you don't need to flush the logs if you're not also spawning a goroutine, because you can rely on your caller to flush those same logs when they return. You could easily envision a synchronous version instead (see below). Unfortunately, the version we tried turns out to be (empirically) too difficult to use correctly.
We started with an API based on keeping the goroutines in user code, along the lines of: // Detached returns a new Context with values derived from parent.
//
// The caller must call the returned "close" function exactly once when the values from
// the new Context are no longer in use by any goroutine.
//
// The values in the new Context are obtained by calling the PreserveFunc for each
// registered key. Keys without a PreserveFunc are not associated with any value in
// the new Context. If the PreserveFunc for a key returns a nil value, the new Context
// will not have a value for that key.
//
// The new Context is never Done, even if the parent Context is canceled or reaches
// its deadline.
func Detached(parent context.Context) (_ context.Context, close func()) Empirically, a significant fraction of attempted users ended up calling |
But how is "and then flush the Context logs" implemented? I assume that's user code, since otherwise I don't know what a Context log is, so "run this function and then flush the Context logs" is really "run this function and then run this other function". How does that avoid generalizing to arbitrary sequences? / Why is it appropriate to stop at two? |
It is 2N+1 calls in the general case:
The pairing of calls before and after is what makes the API interesting: the first batch of N calls must be made while the parent logs have not yet been flushed (so that they can emit links to the child logs), and the last batch of N calls must not be forgotten (so that the child logs are actually written). In the general case, a library function receiving a |
OK, I reread this whole thread carefully, and while I think I understand the problem being solved, it seems like a solution tailored to a very specific problem. Is there any evidence that this is a widespread need? Is there any evidence that there aren't N more of these that need solving as well? It just feels like "I have this precisely-shaped problem hole and this precisely-shaped solution exactly fits into that hole exactly." Usually instead we look for solutions that fit an entire class of problem holes. This proposal still seems far too complex and specific, not like typical Go solutions. Also, @Sajmani proposed a different solution and in response @zombiezen wrote in #19643 (comment):
If (1) Sameer's proposal satisfies the goal and (2) Sameer's proposal can be written without any help from the context package, why not start there? Introduce a new self-contained detach package in your own code base, use it, and learn from it whether there needs to be anything at all in the context package proper. |
@rsc @Sajmani's solution is not just the |
OK, well the main point is the part above the fold in my reply: this seems overfitted to the specific problem being solved. |
@zombiezen @rsc My proposal is specifically to move any registry to a separate package and not the context package. I don't want global state like the registry in the context package, so that it's easier to permit multiple versions of the context package to coexist in a program. Packages with global state need to be singly-versioned in a program, so best to keep them focused on that state. Also, while |
I talked some more with @Sajmani about this, and we both believe that there just isn't enough experience with what's needed here to commit to something in the standard library today. As such, I am going to close this proposal as declined. This is what I see as the path forward here:
Thanks. |
(not reopening the issue) In an earlier comment, I proposed defining this interface: It occurred to me that the ability to separate a context's values from the rest of the context (notably its deadline and cancelation) is also useful in logging and tracing. Some logging APIs want values from a context, but it is somewhat confusing to pass a Context to a Log call, since this suggests that the Log call might be canceled. With a Values interface, we can define: |
@neild and I tossed this exact idea around a few months ago when looking at logging APIs. Things to consider:
(And while I'm here: thanks @rsc, the resolution here seems reasonable.) |
Summary
A common need when writing server code is performing a task that intentionally goes past the deadline of the request while preserving the request's context values (e.g. trace ID). These tasks may block a request handler from returning (synchronous) or may need to happen in the background, past the return of a request handler (asynchronous). Creating a Context that obtains values from a parent context correctly is difficult in the asynchronous case because:
I would like to add these functions to the
context
package (but happy to start withx/net/context
):/cc @bcmills @rakyll
Use Cases
Making another RPC to clean up a resource (like rolling back a transaction) after receiving a cancel. This would likely be a synchronous action (an
IgnoreCancel
usage).Wanting to log or update a counter after servicing a request but not wanting to block the response. This would be an asynchronous action (a
Detach()
usage).The text was updated successfully, but these errors were encountered: