Skip to content
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: reconsider semantics for AfterFunc stop function #60156

Closed
rogpeppe opened this issue May 12, 2023 · 9 comments
Closed

proposal: context: reconsider semantics for AfterFunc stop function #60156

rogpeppe opened this issue May 12, 2023 · 9 comments

Comments

@rogpeppe
Copy link
Contributor

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
Copy link
Contributor

neild commented May 12, 2023

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.

@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 12, 2023
@seankhliao seankhliao added this to the Go1.21 milestone May 12, 2023
@rogpeppe
Copy link
Contributor Author

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
Copy link
Contributor

rsc commented May 17, 2023

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.

@rsc rsc changed the title context: reconsider semantics for AfterFunc stop function proposal: context: reconsider semantics for AfterFunc stop function May 17, 2023
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 17, 2023
@rogpeppe
Copy link
Contributor Author

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
Copy link
Contributor

neild commented May 17, 2023

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
Copy link
Contributor

rsc commented May 17, 2023

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
Copy link
Contributor

rsc commented May 24, 2023

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

@rsc
Copy link
Contributor

rsc commented May 24, 2023

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

@rsc
Copy link
Contributor

rsc commented May 31, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Declined
Development

No branches or pull requests

6 participants