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: time: context-aware time Ticker and Timer #45571

Closed
alercah opened this issue Apr 14, 2021 · 13 comments
Closed

proposal: time: context-aware time Ticker and Timer #45571

alercah opened this issue Apr 14, 2021 · 13 comments

Comments

@alercah
Copy link

alercah commented Apr 14, 2021

Currently, time.Ticker and time.Timer are not context-aware. A typical context-aware usage of a time.Ticker might look like this:

ticker := time.NewTicker(time.Second)
defer ticker.Stop()

for {
  select {
    case val := <-ticker.C:
      // Do things
    case <-ctx.Done():
      return ctx.Err()
  }
}

But it could look like this:

ticker := time.NewTickerCtx(ctx, time.Second)
for val := range <-ticker.C {
  // Do things
}

The context-aware Timer and Ticker would automatically stop and close the channel when the context is cancelled. The channel must be closed, unlike with Stop(), or else the client would still need to check for the context being cancelled. With cancellation via select, there is no risk of a spurious last tick as there is with Stop(). For consistency, I propose that Stop() should retain its existing behaviour, even for timers/tickers created with NewTimerCtx/NewTickerCtx.

@gopherbot gopherbot added this to the Proposal milestone Apr 14, 2021
@ianlancetaylor ianlancetaylor changed the title proposal: context-aware time Ticker and Timer proposal: time: context-aware time Ticker and Timer Apr 14, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 14, 2021
@ianlancetaylor
Copy link
Contributor

The time package can't depend on the context package, as that would introduce a cyclical import. But perhaps these could be added to the context package?

@smasher164
Copy link
Member

smasher164 commented Apr 15, 2021

The context-aware Timer and Ticker would automatically stop and close the channel when the context is cancelled.

This would mean the Stop and Reset methods of *Timer would no longer be usable. Edit: And would make this a single-use timer when constructed with a context.

@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

For both the reasons mentioned, it sounds like this should be a separate API from the ones in package time.
You could imagine

package context
func NewTicker(ctx Context, d time.Duration) <-chan time.Time

But it seems like that could be provided just as easily in a third-party package?

@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

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 rsc moved this from Incoming to Active in Proposals (old) Apr 21, 2021
@nightlyone
Copy link
Contributor

context.Sleep(ctx context.Context, d time.Duration) also proved quite useful in multiple projects to ensure a minimum quiet time but still allow quick exits of services. I would appreciate to have all three available.

I believe they are a good addition to the context package since it nicely extends the relationship between context and time by periodic things, solves problems that won't exist without context.

Having them in the standard library avoids repeating tricky code in multiple projects as well as allowing optimizations that can react to timers never firing.

@alercah
Copy link
Author

alercah commented Apr 22, 2021

The time package can't depend on the context package, as that would introduce a cyclical import. But perhaps these could be added to the context package?

Ah, that's a shame. An alternative would be if time redeclared the interface to break the dependency loop?

This would mean the Stop and Reset methods of *Timer would no longer be usable. Edit: And would make this a single-use timer when constructed with a context.

I don't see any issue with Reset continuing to work as-is. Stop (for both Ticker and Timer) is possibly trickier, but I think the easiest thing to do would be to leave them with their current behaviour, unless we want to make new types for them.

But it seems like that could be provided just as easily in a third-party package?

A third-party package would be an option, but it's actually more work than you might expect to write this in terms of the existing time primitives. And I think that Go APIs should generally be context-aware, so in some respects the lack of context-aware timing utilities is a flaw in the stdlib.

@smasher164
Copy link
Member

I don't see any issue with Reset continuing to work as-is. Stop (for both Ticker and Timer) is possibly trickier, but I think the easiest thing to do would be to leave them with their current behaviour, unless we want to make new types for them.

Correct me if I'm wrong, but I don't think Timer is allowed to reassign to its channel C.

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
timer := time.NewTimerCtx(ctx, time.Minute)
ch := timer.C
<-ch // context cancelled after 1s, so channel is closed.
if !timer.Stop() {
	<-ch // we won't hit this, since Stop() returned true
}
timer.Reset(time.Minute) // has to reassign C, so it'll send on a completely different channel
<-ch // returns zero value immediately, not actual timestamp after 1 minute

This snippet may not be idiomatic, since t.C is documented in all the Timer examples. But consider the case of a function that purely consumes channel and not a timer. It would not be able to read new input on the channel.

@alercah
Copy link
Author

alercah commented Apr 22, 2021

Reset() should not allow the Timer to be reused, because the context is still expired. Reset() would only work as long as the context has not yet expired.

@bcmills
Copy link
Contributor

bcmills commented Apr 22, 2021

Reset could be defined to reassign the channel if (and only if) the timer was created using time.NewTimerCtx, but at that point I don't see much advantage in having Reset at all vs. just allocating a new timer. So probably it's better to have NewTimerCtx return a type that doesn't have a Reset method.

(See also #37196.)

@smasher164
Copy link
Member

Reset() would only work as long as the context has not yet expired.

I see. That's interesting. So it would be like a Timer that's destructed when the context expires, except we still have a handle to the Timer.

The only thing that concerns me is that because Timer currently doesn't close its channel, there might be code that subtly breaks if it loops indefinitely reading from its channel, since the consumer can't know if it was created with a context. It's less likely to happen with Timer, but could be more of an issue with Ticker.

@rsc
Copy link
Contributor

rsc commented Apr 28, 2021

Re time and context, Go packages form a strict import hierarchy. Time is an incredibly low-level package. Context is much higher level. So time cannot refer to context, unfortunate though that may seem.

Re the broader discussion, all these detailed arguments strongly suggest that this API should be worked out in a third-party package first before we consider adding new API to context itself.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) May 5, 2021
@rsc
Copy link
Contributor

rsc commented May 5, 2021

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

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) May 12, 2021
@rsc
Copy link
Contributor

rsc commented May 12, 2021

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

@rsc rsc closed this as completed May 12, 2021
@golang golang locked and limited conversation to collaborators May 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants