time timer.Reset is not thread safe

[majek]

Yes, I am aware that timer.Reset is impossible to be used correctly. This bug is not about timer.Reset semantics, it's about panic it causes.

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

The bug happens in 1.10. The bug doesn't happen in 1.9.7

marek@:~$ go version
go version go1.10-3 CF linux/amd64

Does this issue reproduce with the latest release?

Haven't tried. Looking at code I'd say - yes.

What did you do?

package main

import (
	"time"
)

func main() {
	tm := time.NewTimer(1 * time.Millisecond)
	for i := 0; i < 2; i++ {
		go func() {
			for {
				tm.Reset(1 * time.Millisecond)
			}
		}()
	}
	for {
		<-tm.C
	}
}

What did you expect to see?

Not a panic.

What did you see instead?

A panic

marek@:~$ GOMAXPROCS=20 go run a.go
panic: runtime error: index out of range
fatal error: panic holding locks

goroutine 6 [running]:
runtime.throw(0x47bcfc, 0x13)
        /usr/local/go/src/runtime/panic.go:619 +0x81 fp=0xc420074e08 sp=0xc420074de8 pc=0x422f11
panic(0x469ae0, 0x4bcbb0)
        /usr/local/go/src/runtime/panic.go:465 +0x4b0 fp=0xc420074ea8 sp=0xc420074e08 pc=0x422b60
runtime.panicindex()
        /usr/local/go/src/runtime/panic.go:28 +0x5e fp=0xc420074ec8 sp=0xc420074ea8 pc=0x42171e
runtime.siftupTimer(0xc42000e080, 0x2, 0x2, 0x2)
        /usr/local/go/src/runtime/time.go:331 +0xed fp=0xc420074ed8 sp=0xc420074ec8 pc=0x43befd
runtime.(*timersBucket).addtimerLocked(0x4c1f20, 0xc4200a8008)
        /usr/local/go/src/runtime/time.go:146 +0xad fp=0xc420074f30 sp=0xc420074ed8 pc=0x43b66d
runtime.addtimer(0xc4200a8008)
        /usr/local/go/src/runtime/time.go:131 +0x83 fp=0xc420074f60 sp=0xc420074f30 pc=0x43b593
time.startTimer(0xc4200a8008)
        /usr/local/go/src/runtime/time.go:111 +0x2b fp=0xc420074f78 sp=0xc420074f60 pc=0x43b4ab
time.(*Timer).Reset(0xc4200a8000, 0xf4240, 0x1)
        /usr/local/go/src/time/sleep.go:130 +0x83 fp=0xc420074fb0 sp=0xc420074f78 pc=0x455073
main.main.func1(0xc4200a8000)
        /home/marek/a.go:12 +0x34 fp=0xc420074fd8 sp=0xc420074fb0 pc=0x459c04
runtime.goexit()
        /usr/local/go/src/runtime/asm_amd64.s:2361 +0x1 fp=0xc420074fe0 sp=0xc420074fd8 pc=0x449b71
created by main.main
        /home/marek/a.go:10 +0x5a

goroutine 1 [runnable]:
main.main()
        /home/marek/a.go:17 +0x83

goroutine 5 [runnable]:
time.startTimer(0xc4200a8008)
        /usr/local/go/src/runtime/time.go:111 +0x2b
time.(*Timer).Reset(0xc4200a8000, 0xf4240, 0x1)
        /usr/local/go/src/time/sleep.go:130 +0x83
main.main.func1(0xc4200a8000)
        /home/marek/a.go:12 +0x34
created by main.main
        /home/marek/a.go:10 +0x5a
exit status 2

Explanation

timer.Reset goes into addtimer
https://github.com/golang/go/blob/go1.10/src/runtime/time.go#L128-L133

func addtimer(t *timer) {
	tb := t.assignBucket()
	lock(&tb.lock)
	tb.addtimerLocked(t)
	unlock(&tb.lock)
}

See that t.assignBucket is called outside of lock. My understanding is that the result of assignBucket depends based on current thread. Therefore we can end up in timer being assigned to different tb if they were called concurrently.

This isn't a problem itself, but then addTimerLocked sets t.i:

	t.i = len(tb.t)

which of course makes no sense if it's done by diffent buckets. Therefore golang crashes with index error.

[ALTree]

Dup of #25686, fixed on tip (to be 1.11), your snippet now crashes with:

panic: runtime error: racy use of timers

goroutine 18 [running]:
time.startTimer(0xc000080008)
	/home/alberto/go/src/runtime/time.go:114 +0x2b
time.(*Timer).Reset(0xc000080000, 0xf4240, 0x1)
	/home/alberto/go/src/time/sleep.go:130 +0x81
main.main.func1(0xc000080000)
	/home/alberto/Desktop/prova.go:12 +0x34
created by main.main
	/home/alberto/Desktop/prova.go:10 +0x5a
exit status 2

[majek]

Ok, I don't understand the relationship between the timer.Reset panicking and inability to use its return value in sensible way.

The docs say:
https://golang.org/pkg/time/#Timer.Reset

This should not be done concurrent to other receives from the Timer's channel.

The docs don't say that the runtime will panic if you call timer.Reset from two goroutines.

Perhaps I'l explain my use case:

• I keep a timer for a connection
• I reset it on read() or write()
• so I want the timer to expire after X seconds of last activity
• (A) I really really really don't care what happens in the corner case BOTH read/write happens AND timer expires. I'm happy for that case to be undefined.
• (B) But I really really care that when both read AND write happen at the same time, I could call timer.Reset() from two different goroutines.

Right now the semantics of timer.Reset seem to focus on documenting (A) while ignoring the way more common pattern (B).

To get (B) working I need to... dunno... have a lock around timer.Reset()? This is pretty insane.

[ALTree]

Can you open a new issue? This one was about the panic and I think we fixed that. I don't understand if you're asking for a doc change, a behaviour change, or both. I also don't understand the connection between the bad-looking panic you reported initially and your second post. In the new issue, please explain clearly what kind of change you're asking for. Thanks.