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: add a channel to time.Ticker to detect stopped tickers #35643

Closed
AnasHSulaiman opened this issue Nov 17, 2019 · 8 comments

Comments

@AnasHSulaiman
Copy link

@AnasHSulaiman AnasHSulaiman commented Nov 17, 2019

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

go1.13.4 

Does this issue reproduce with the latest release?

Yes

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

darwin, amd64

What did you do?

https://play.golang.org/p/e5B2M9b1rWc

What did you expect to see?

I would like a simple way to detect a stopped ticker.

I know it's easily implemented using a second channel: https://play.golang.org/p/r36mEd85_6S

However, In a service where tickers are created and stopped frequently, it's not elegant (to say the least) to create a second channel with each ticker to avoid memory leaks when tickers are stopped (https://play.golang.org/p/lGfYd7Kq3kH).

I have read the discussion in #2650 and understand it is preferred to keep current semantics.

However, something like the following would make things easier without breaking existing semantics:

ticker := time.NewTicker(1 * time.Second)
go func() {
	for {
		select {
		case <- ticker.C:
			fmt.Println("tick")
		case <- ticker.Stopped:
			return
		}
	}
}()
@gopherbot gopherbot added this to the Proposal milestone Nov 17, 2019
@gopherbot gopherbot added the Proposal label Nov 17, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 17, 2019

Seems that you could define your own type that uses time.Ticker to implement the semantics you want.

@ianlancetaylor ianlancetaylor changed the title Proposal: Another channel in time.Ticker to detect stopped tickers proposal: time: add a channel to time.Ticker to detect stopped tickers Nov 17, 2019
@AnasHSulaiman

This comment has been minimized.

Copy link
Author

@AnasHSulaiman AnasHSulaiman commented Nov 17, 2019

Yep. That's what I did.

Considering that this issue has come up a number of times (#33831, #4366, #2650), I thought the community would benefit from a built in solution.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 17, 2019

Adding a channel to the time.Ticker type isn't free, and experience seems to suggest that relatively few people need it. A go-gettable package seems like the way to go for now.

I don't think your issue citations refer to your suggestion. Those issues are all suggesting that when a time.Ticker is stopped it should close the ticker channel.

@AnasHSulaiman

This comment has been minimized.

Copy link
Author

@AnasHSulaiman AnasHSulaiman commented Nov 17, 2019

Good point. I thought a channel of type chan struct{} is of size zero, but I was wrong: https://play.golang.org/p/LbBtcaK9UJT

As to the citations, my bad. I meant to say that this proposal would be an alternative solution to cited issues.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

It is the job of the standard library to provide strong, robust building blocks like time.Ticker.
It is not the job of the standard library to provide every possible extension to those building blocks. Doing so would make the standard library more difficult to learn, to maintain, and in many cases also to use.

This does not seem to come up often enough to be worth the added complexity and runtime cost of a second channel in the ticker interface for every user. If your use case really needs to know when the ticker has been stopped, then as has been pointed out it is easy to write and use a wrapper that provides that extra functionality.

@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

Given that this is very easy to do anyhow and not possible to do in time.Ticker without forcing overhead on everyone, this seems like a likely decline.

Leaving open for a week for final comments.

@AnasHSulaiman

This comment has been minimized.

Copy link
Author

@AnasHSulaiman AnasHSulaiman commented Nov 27, 2019

Good with me. I didn't consider the the size of the channel when I submitted this proposal.
The memory leak aspect of this issue still stands. A goroutine blocked on a stopped ticker will hold on to a reference to that ticker for the lifetime of the service.

@rsc rsc moved this from Incoming to Likely Decline in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

No change in consensus, so declining.

@rsc rsc closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Likely Decline
4 participants
You can’t perform that action at this time.