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: Timer.StopAndClear method #38945

Closed
zeripath opened this issue May 8, 2020 · 6 comments
Closed

proposal: time: Timer.StopAndClear method #38945

zeripath opened this issue May 8, 2020 · 6 comments
Labels
Projects
Milestone

Comments

@zeripath
Copy link

@zeripath zeripath commented May 8, 2020

The correct formulation for handling for time.Timer.Stop() is error prone and awkward.

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

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/andrew/.cache/go-build"
GOENV="/home/andrew/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/andrew/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/andrew/src/go/gitea/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build840805710=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In several places in our codebase we create time.Timers. The problem is that handling the timer stop condition is not obvious to the beginning user and, even for those who know to clear the channel the correct incantation is errorprone (do I potentially need to clear the channel !timer.Stop() or timer.Stop()) and is needlessly repetitive.

What did you expect to see?

There should be a simple function that will stop and clear the channel for the timer.

e.g.

func (timer *time.Timer) StopAndClear() {
	if !timer.Stop() {
		select {
		case <-timer.C:
		default:
		}
	}
}

What did you see instead?

Every time I stop a timer I have to write:

timer := time.Timer(...)
...
if !timer.Stop() {
	select {
	case <-timer.C:
	default:
	}
}

to ensure that the channel does not leak. I also have to remember that the block has to activate when not stopped. Reviewers of code have to all be aware of this too.

It seems to be a needless gotcha that could be handled relatively nicely in the standard library.

I would also advise that the formulation in the standard library documentation is also changed as it encourages the naïve user to cause a deadlock:

i.e. instead of

if !t.Stop() {
	<-t.C
}

write

if !t.Stop() {
	select {
	case <-t.C:
	default:
	}
}

Thanks so much for the language in general though!

@gopherbot gopherbot added this to the Proposal milestone May 8, 2020
@gopherbot gopherbot added the Proposal label May 8, 2020
@ianlancetaylor ianlancetaylor changed the title Proposal: time.Timer.StopAndClear() proposal: time: Timer.StopAndClear method May 8, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 8, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 8, 2020

I'm concerned that this might be a bit of a footgun, since correct behavior really depends on whether there are any other goroutines also reading from the channel. If you know for sure that there are no other goroutines reading, then the code in https://golang.org/pkg/time/#Timer.Stop is correct:

    if !t.Stop() {
        <-t.C
    }

If there are other goroutines reading from the channel, then it's not completely obvious what the right code is. At least, it's not obvious to me.

Just to be very clear, there is no need to read from the channel after calling t.Stop in the general case. The only time this case arises is if you want to keep using the timer, presumably by calling t.Reset.

@bcmills
Copy link
Member

@bcmills bcmills commented May 11, 2020

Note that

	if !t.Stop() {
		select {
		case <-t.C:
		default:
		}
	}

is not entirely reliable today (with or without concurrent receivers) due to #37196.

(If that issue is addressed, then that pattern will be correct even in the presence of concurrent receivers.)

@rsc
Copy link
Contributor

@rsc rsc commented May 20, 2020

I'm not sure that StopAndClear has any possible benefit. At first it might seem like it does, but the details of concurrency and channels and goroutines make the situation more subtle than it first appears.

If another goroutine is waiting to read a value from a channel, then t.StopAndClear would race with it. The t.StopAndClear call could return but then the other goroutine could still - in physical wall time - receive the value afterward, due to scheduling delays. You need some explicit synchronization with the other goroutine to make sure that processing of the receive is over. But if you are going to that trouble (or you don't have the other goroutine) then t.Stop is sufficient.

So t.StopAndClear doesn't end up being any easier to use than t.Stop.

The one thing t.StopAndClear could do is solve the "time value waiting in a buffered channel" issue. But if we want to fix that, we should probably just make t.Stop do that part, instead of adding new API. That's #37196 (thanks Bryan), which we can discuss over there.

@rsc
Copy link
Contributor

@rsc rsc commented May 27, 2020

Based on the discussion above, this seems like a likely decline.
(Looks like I aded the Proposal-FinalCommentPeriod label incorrectly last week; the project status "Active" was correct.)

@rsc rsc moved this from Active to Likely Decline in Proposals May 27, 2020
@zeripath
Copy link
Author

@zeripath zeripath commented May 27, 2020

Yeah it looks like I was confused by the documentation - it's nice to know that we don't need to clear channels to prevent leaks.

@zeripath zeripath closed this May 27, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 3, 2020

No change in consensus (and retracted), so marking declined.

@rsc rsc moved this from Likely Decline to Declined in Proposals Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

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