-
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
context: add AfterFunc #57928
Comments
Change https://go.dev/cl/462855 mentions this issue: |
It is worth noting that
|
The // Try executes fn while watching ctx. If ctx is cancelled, Try calls cancel and returns ctx.Err().
// Otherwise, Try returns the result of fn.
// This is implemented using an internal implementation of OnDone as described above.
func Try(fn func() error, cancel func()) error
// Example of using Try.
func ContextReadOnDone(ctx context.Context, conn net.Conn, b []byte) (n int, err error) {
err = context.Try(func() error {
n, err = conn.Read(b)
}, func() {
conn.SetReadDeadline(time.Now())
})
return n, err
} |
func Try(ctx context.Context, fn func() error, cancel func()) error {
stopf := context.OnDone(ctx, cancel)
err := fn()
stopf()
if ctx.Err() != nil {
return ctx.Err()
}
return err
} |
The
|
Based on that analogy, I would suggest a slightly different implementation of func Try(ctx context.Context, fn func() error, cancel func()) error {
canceled := false
stop := context.OnDone(ctx, func() {
cancel()
canceled = true
})
err := fn()
stop()
if canceled && err == nil {
return ctx.Err()
}
return err
} Notably: in case of cancellation I would prefer to still return the result of |
This proposal has been added to the active column of the proposals project |
This seems similar to having time.AfterFunc to avoid making a goroutine that calls time.Sleep and then the function. Here we avoid making a goroutine that receives from ctx.Done and then calls the function. Perhaps it should be context.After or AfterFunc?
reads nicely to me. It's nice that this would let people implement context.Merge themselves at no efficiency cost compared to the standard library. We should specify that f is always run in a goroutine by itself, at least semantically, even in the case where
f shouldn't be called by context.After in that case either. |
I like |
|
This may be implied, but to clarify since I don't see it in the example... |
OK, so it sounds like the signature is
AfterFunc arranges to call f after ctx is done (cancelled or timed out), and it calls f in a goroutine by itself. Even if ctx is already done, calling AfterFunc does not wait for f to return. Multiple calls to AfterFunc on a given ctx are valid and operate independently; one does not replace another. Calling stop stops the association of ctx with f. It reports whether the call stopped f from being run. (This last paragraph is adapted from time.Timer.Stop.) Do I have that right? Anything wrong there? |
I'd prefer to have calling |
I find this sentence a bit off when I read it. If I understand the proposal, calling AfterFunc never waits for f to return. The above sentence seems to emphasize the corner case of ctx already being done, though. That emphasis makes it seem like there is a special case involved, but there really isn't. Consider rewording to:
I think this better emphasizes that the behavior is always the same while still pointing out the less than obvious corner case. |
While naming returned func |
If f is long-running, then you'd need to make arrangements to interrupt it. If you do want a long-running f, and a
This is less convenient than a non-blocking stop (and a bit less efficient), but I think it's also the less common case. Every motivating example I've come up for A non-blocking stop makes it easier to inadvertently leak a long-running operation. In general, functions should clean up any goroutines they start before returning. A blocking stop ensures that the AfterFunc isn't left running unless the programmer takes specific steps to create a goroutine for it, as above. A non-blocking stop can also make the common case quite a bit more subtle, and possibly less efficient. Taking the case of reading from a
|
@neild your second example is great, but first one looks very race-prone:
TBH I don't remember outcome from last change in https://go.dev/ref/mem (i.e. is it safe to read/write int-sized vars like bool), but even if it's safe callback can be started by context.AfterFunc but didn't execute it's first operation |
@powerman The first example assumes a |
But isn't blocking |
A blocking |
A blocking stop can easily lead to deadlocks, especially if these functions are trying to send on channels to notify other goroutines that the context is cancelled. A non-blocking stop won't, and it matches time.Timer.Stop. I'm not entirely sure how to decide between those benefits and the ones @neild has pointed out. |
Perhaps a non-blocking stop to match time.Timer.Stop, plus the Try function proposed by @ianlancetaylor above to handle the cases where you want to synchronize on the AfterFunc?
Alternatively, |
No change in consensus, so accepted. 🎉 |
One thing that wasn't discussed yet is how this proposal interact with #40221 (context.WithoutCancel)? IIUC, the function supplied in AfterFunc is still called, which means that using AfterFunc to do anything with context values (like closing a trace span) is going to be problematic if context.WithoutCancel is ever used. I know that it's possible to do everything that context.AfterFunc is doing, but I wonder if it's going to be more attractive to assume that context's have a life-cycle that can be hooked into, while at the same time a method exists to escape a context from that life-cycle. Internally at Google, we have a solution that adds a different life-cycle hook that to do work during context detaching (e.g. to open a new trace span for a background go routine), assuming the right context detaching method is used. It's probably worth a separate proposal, but I do wonder if the implementation of this proposal is going to make it more complicated to add detaching functionality later. To be clear, I don't know the answer to that question. |
|
Got it. Do you think it might makes sense to be explicit about the fact that the function might never be called? That might dissuade its use in cases like this: func dispatchRequest(ctx context.Context) {
ctx, cancel = context.WithCancel(ctx)
defer cancel()
handleRequest(ctx)
}
func handleRequest(ctx context.Context) {
var span trace.Span
ctx, span = tracer.Start(ctx, "operation")
context.AfterFunc(ctx, span.End)
backgroundAction(ctx)
}
func backgroundAction(ctx context.Context) {
ctx = context.WithoutCancel(ctx)
go someLibraryFunc(ctx)
}
func someLibraryFunc(ctx context.Context) {
span = trace.SpanFromContext(ctx) // <--- span likely ended already, updates are not allowed
} Arguably, this issue already exists today. I am wondering if it will be worse, because it's too easy to assume that |
Change https://go.dev/cl/486535 mentions this issue: |
For #40221 For #56661 For #57928 Change-Id: Iaf7425bb26eeb9c23235d13c786d5bb572159481 Reviewed-on: https://go-review.googlesource.com/c/go/+/486535 Run-TryBot: Damien Neil <dneil@google.com> Reviewed-by: Sameer Ajmani <sameer@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
I only just saw this issue; sorry for the late comment. I'm not entirely sure whether the semantics of An alternative semantic could be:
That makes it feasible to call I'm wondering what the use case is for knowing whether this is the |
@rogpeppe Thanks, I suggest that you open that in a new issue, as this semantics has already been implemented. We can make the new issue a release blocker for 1.21. Thanks. |
@ianlancetaylor Thanks. Done. |
Edit: The latest version of this proposal is #57928 (comment).
This proposal originates in discussion on #36503.
Contexts carry a cancellation signal. (For simplicity, let us consider a context past its deadline to be cancelled.)
Using a context's cancellation signal to terminate a blocking call to an interruptible but context-unaware function is tricky and inefficient. For example, it is possible to interrupt a read or write on a
net.Conn
or a wait on async.Cond
when a context is cancelled, but only by starting a goroutine to watch for cancellation and interrupt the blocking operation. While goroutines are reasonably efficient, starting one for every operation can be inefficient when operations are cheap.I propose that we add the ability to register a function which is called when a context is cancelled.
OnDone
permits a user to efficiently take some action when a context is cancelled, without the need to start a new goroutine in the common case when operations complete without being cancelled.OnDone
makes it simple to implement the merged-cancel behavior proposed in #36503:Or to stop waiting on a
sync.Cond
when a context is cancelled:The
OnDone
func is executed in a new goroutine rather than synchronously in the call toCancelFunc
that cancels the context because context cancellation is not expected to be a blocking operation. This does require the creation of a goroutine, but only in the case where an operation is cancelled and only for a limited time.The
CancelFunc
returned byOnDone
both provides a mechanism for cleaning up resources consumed byOnDone
, and a synchronization mechanism. (See theContextReadOnDone
example below.)Third-party context implementations can provide an
OnDone
method to efficiently scheduleOnDone
funcs. This mechanism could be used by thecontext
package itself to improve the efficiency of third-party contexts: Currently,context.WithCancel
andcontext.WithDeadline
start a new goroutine when passed a third-party context.Two more examples; first, a context-cancelled call to
net.Conn.Read
using the APIs available today:And with
context.OnDone
:The text was updated successfully, but these errors were encountered: