proposal: context: reconsider semantics for AfterFunc stop function

[rogpeppe]

go version devel go1.21-3ed8a1e629 Tue Mar 28 05:41:44 2023 +0000 linux/amd64

I raised this point in #57928.

I'm not entirely sure whether the semantics of the stop return from context.AfterFunc are correct. The argument for it being this way is by reference to Timer.Stop but I'm not convinced that Timer.Stop is good precedent, because that API is notoriously error-prone and hard to use.

An alternative semantic could be:

// The stop function reports whether the function has been successfully stopped; that is, it returns false
// if and only if the function has been invoked already.

That makes it feasible to call stop multiple times concurrently and have consistent results between them, and it seems to me like a simpler invariant to explain.

I'm wondering what the use case is for knowing whether this is the stop call that prevented the function running.

[neild]

I would phrase the proposed alternative semantic as something like:

// The stop function reports whether the function has been successfully stopped.
// If stop returns true, the function will not be run.
// If it returns false, the function has run or will be.

That is, the function may not have been invoked yet, but if stop returns false it definitely will be.

I like these semantics better than the time.Timer-based semantics, but I'm not certain whether any improvement is worth the inconsistency. Having two different AfterFuncs with subtly different semantics seems unfortunate.

[rogpeppe]

Having two different AfterFuncs with subtly different semantics seems unfortunate.

I agree that's somewhat unfortunate, but the time.Timer semantics are already unfortunate, and I'm hoping we can avoid compounding that misfortune here.

[rsc]

The discussion in #57928 around stop focused on whether stop blocks until f is done, not on what happens when stop is called multiple times. We didn't directly discuss what the second call to stop should return.

If you think that f is going to do something important that needs to happen exactly once, then this pattern

ctx, stop := context.AfterFunc(ctx, f)
...
if stop() {
    do the important thing f didn't
}

is well supported by stop returning true from only one of its concurrent calls.

The time.Timer API is confusing because if you didn't get f in time you need to do cleanup (<-t.C), but only once, and the result doesn't actually help with that. But that specific cleanup won't happen with AfterFunc. (That is, the confusing part about Timer is when you're using the channel form, not the AfterFunc form.)

Is there a use case for context.AfterFunc where these stop semantics are problematic? Both of the links in the top message here are about time.Timer.C.

[rogpeppe]

The discussion in #57928 around stop focused on whether stop blocks until f is done, not on what happens when stop is called multiple times. We didn't directly discuss what the second call to stop should return.

If you think that f is going to do something important that needs to happen exactly once, then this pattern

ctx, stop := context.AfterFunc(ctx, f)
...
if stop() {
    do the important thing f didn't
}

If you think that f is going to do something important that needs to happen exactly once, then I'd suggest that this pattern:

var once sync.Once
cleanupOnce := func() {
   once.Do(cleanup)
})
ctx, stop := context.AfterFunc(ctx, f)
// using my proposed behaviour for stop
if !stop() {
    cleanupOnce()
}

is probably sufficient and fairly natural.

On the other hand, finding out whether the function has run (or is about to run) is a little more awkward with the original semantics:

result := make(chan error, 1)
f := func() {
   result <- doSomething()
}
ctx, stop := context.AfterFunc(ctx, f)

// In some other goroutine, using originally proposed behaviour for stop.
if stop() {
   // Are we expecting a value inside the result channel or not?
   // If so:
  err := <-result
  result <- err  // Make available for other goroutines to see.
}

The issue is that the function might be about to be run and we can't tell that from the result of stop.

I guess that's possible to work around by wrapping stop itself:

func stopper(stop func() bool) func() bool {
	actuallyStopped := false
	var once sync.Once
	return func() bool {
		once.Do(func() {
			actuallyStopped = stop()
		})
		return actuallyStopped
	}
}

So both semantics are implementable in terms of the other.

I happen to think that the behaviour I suggested is a bit more intuitive and easily explained, as it makes the return from stop mean the same regardless of how many times it's been called: if it returns false then the function did get called and you have to deal with the consequences.

[neild]

if it returns false then the function did get called and you have to deal with the consequences.

This is subtly but importantly not correct: If stop returns false, then the AfterFunc has been or will be called. It might not have been called yet, or it may still be running.

I think that the common use for the return value of stop is going to be to ask whether we need to wait for the AfterFunc to stop running before cleaning up after it. For example, the ContextReadOnDone example from #57928 (comment) uses stop to decide whether it needs to reset the connection deadline.

I'm not certain under what circumstances we might call stop more than once, so it's hard for me to say what semantics are most useful. I find "stop reports whether the function will not run" more intuitive than "stop reports whether this specific call to stop caused the function to not run", but this is all subtle enough that consistency with time.AfterFunc seems less confusing overall.

[rsc]

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

[rsc]

Based on the discussion above, it sounds like we should leave it matching AfterFunc.

[rsc]

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

[rsc]

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