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

time: add Ticker.Reset #33184

Closed
namewxt1220 opened this issue Jul 19, 2019 · 9 comments
Closed

time: add Ticker.Reset #33184

namewxt1220 opened this issue Jul 19, 2019 · 9 comments

Comments

@namewxt1220
Copy link

@namewxt1220 namewxt1220 commented Jul 19, 2019

What version of Go are you using (go version)?

go 1.12

Does this issue reproduce with the latest release?

yes.

What operating system and processor architecture are you using (go env)?

linux amd64

What did you do?

I added the api reset function.

What did you expect to see?

I hope that the proposal will take effect,because sometimes we need to adjust the time dynamically.
This is not a problem,this is just a suggestion.
Please read the following.thanks.

api:

// Reset duration to reset a ticker.
func (t *Ticker) Reset(d Duration) bool {
	if t.r.f == nil {
		panic("time: Reset called on uninitialized Ticker")
	}
	w := when(d)
	active := stopTimer(&t.r)
	t.r.when = w
	t.r.period = int64(d)
	startTimer(&t.r)
	return active
}
@agnivade agnivade changed the title update time/tick.go add api proposal: time: add Reset api Jul 19, 2019
@gopherbot gopherbot added this to the Proposal milestone Jul 19, 2019
@gopherbot gopherbot added the Proposal label Jul 19, 2019
@rsc
Copy link
Contributor

@rsc rsc commented Aug 20, 2019

It seems reasonable to be able to change the duration on a ticker.
But the return value from time.Timer.Reset was a mistake and is impossible to use correctly, and this API would provide the same return value. It seems like better options would be to have no return value from Reset and either:

  1. Require that the ticker is stopped; panic if not.
  2. If the ticker is not stopped, first stop it and drain any existing value from the channel.

The second seems defensible to me and useful in code that is doing something like:

t := time.NewTicker(d)
for range t.C {
    if ... { t.Reset(newD) }
}

In general, if the Reset is happening separate from the channel receive, you'd want to do t.Stop, then synchronize with the receiver so it knows to expect a new duration, then t.Reset. But the pattern above does not need that because the receiver and the Reset caller are the same, and that seems likely to be common.

So I would lean toward option 2. Thoughts?

@namewxt1220
Copy link
Author

@namewxt1220 namewxt1220 commented Aug 21, 2019

@rsc
I agree that Reset has no return value,but I don't know what stopTimer returns error under what conditions,at first I just wanted to separate out two apis,like this:

func NewTicker(d Duration) *Ticker {
	...
	//startTimer(&t.r)
	return t
}
func (t *Ticker) StartTicker() {c
	startTimer(&t.r)
}

This way developers can control their own startup time,later added api Reset,using like this:

ticker := time.NewTicker(d)
go func() {
	for _ = range ticker.C {
		...
	}
}()
...
...
...
if ... {
	ticker.Reset(newD)
}

But this does have a situation where stopTimer returns an error,do you have a better solution?

@rsc rsc added this to Incoming in Proposals Dec 4, 2019
@rsc rsc changed the title proposal: time: add Reset api proposal: time: add Ticker.Reset Jan 15, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jan 15, 2020

Given the two options I mentioned above, it seems like "the ticker is not stopped, first stop it and drain any existing value from the channel" is the much less error-prone API. If you don't want that, you can of course stop the ticker yourself first.

With that clarification, this seems like a likely accept.

@rsc rsc moved this from Incoming to Likely Accept in Proposals Jan 15, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jan 22, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jan 22, 2020
@rsc rsc changed the title proposal: time: add Ticker.Reset time: add Ticker.Reset Jan 22, 2020
@rsc rsc modified the milestones: Proposal, Backlog Jan 22, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 3, 2020

Change https://golang.org/cl/217362 mentions this issue: time: add Ticker.Reset

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 23, 2020

Change https://golang.org/cl/220638 mentions this issue: Revert "time: add Ticker.Reset"

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 23, 2020

CL reverted due to test failures, so reopening.

gopherbot pushed a commit that referenced this issue Feb 23, 2020
This reverts CL 217362 (6e5652b.)

Reason for revert: Causing failures on arm64 bots. See #33184 for more info

Change-Id: I72ba40047e4138767d95aaa68842893c3508c52f
Reviewed-on: https://go-review.googlesource.com/c/go/+/220638
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@andybons
Copy link
Member

@andybons andybons commented Feb 23, 2020

Example failure on arm64: https://build.golang.org/log/518de921e4d47fb5130e0f9f969437fdab9d872e

Relevant snippet:

--- FAIL: TestTicker (4.64s)
    tick_test.go:74: saw 5 errors
    tick_test.go:32: 5 100ms ticks took 967.285125ms, expected [480ms,720ms]
    tick_test.go:32: 5 100ms ticks took 866.8515ms, expected [480ms,720ms]
    tick_test.go:32: 5 100ms ticks took 895.289792ms, expected [480ms,720ms]
    tick_test.go:32: 5 100ms ticks took 901.194458ms, expected [480ms,720ms]
    tick_test.go:32: 5 100ms ticks took 1.009654709s, expected [480ms,720ms]
FAIL
FAIL	time	8.980s
FAIL
go tool dist: Failed: exit status 1
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 23, 2020

Change https://golang.org/cl/220424 mentions this issue: time: add Ticker.Reset

@gopherbot gopherbot closed this in 402ea9e Feb 24, 2020
@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
133 of 133 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.