code review 9035047: runtime: handle timer overflow in tsleep

runtime: handle timer overflow in tsleep

Make sure we never pass a timer into timerproc with
a negative duration since it will cause other timers
to never expire.

Fixes issue 5321.

[ality, 2013-05-07 11:40:29]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[minux1, 2013-05-07 14:08:50]

https://codereview.appspot.com/9035047/diff/2002/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

https://codereview.appspot.com/9035047/diff/2002/src/pkg/runtime/time.goc#new...
src/pkg/runtime/time.goc:70: t.when = 1LL<<63-1;
do you really mean 1LL<<62?

[ality, 2013-05-07 20:18:11]

PTAL

minux.ma@gmail.com once said:
>
https://codereview.appspot.com/9035047/diff/2002/src/pkg/runtime/time.goc#new...
> src/pkg/runtime/time.goc:70: t.when = 1LL<<63-1;
> do you really mean 1LL<<62?

Fixed. My brain uses Go's precedence rules.

Thanks,
  Anthony

[remyoudompheng, 2013-05-18 10:51:42]

ping?

it LGTM

[minux1, 2013-05-18 10:55:52]

LGTM also.

[mikio, 2013-05-18 11:18:12]

Please add a test harness for regression like issue 5439.

[ality, 2013-05-21 00:29:41]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com, remyoudompheng@gmail.com,
mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[ality, 2013-05-21 00:33:54]

Test added. Please review it carefully. It's a bit tricky.

I also moved the overflow detection into addtimer since
other parts of the runtime now use it (netpoll.goc).

  Anthony

[r, 2013-05-21 00:34:34]

LGTM

https://codereview.appspot.com/9035047/diff/14001/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

https://codereview.appspot.com/9035047/diff/14001/src/pkg/runtime/time.goc#ne...
src/pkg/runtime/time.goc:100: // when must never be negative otherwise timerproc
will overflow
s/negative/&;/

https://codereview.appspot.com/9035047/diff/14001/src/pkg/runtime/time.goc#ne...
src/pkg/runtime/time.goc:101: // during it's delta calculation and never expire
other timers.
s/it's/its/

[r, 2013-05-21 00:38:54]

https://codereview.appspot.com/9035047/diff/14001/src/pkg/time/internal_test.go
File src/pkg/time/internal_test.go (right):

https://codereview.appspot.com/9035047/diff/14001/src/pkg/time/internal_test....
src/pkg/time/internal_test.go:38: const timeout = 5 * Millisecond
is this long enough for slow machines? i fear flakiness ahead.

i'd make the "timeout" very long - like a second, and the timer very fast, like
1ms.

[ality, 2013-05-21 01:35:08]

PTAL

https://codereview.appspot.com/9035047/diff/14001/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

https://codereview.appspot.com/9035047/diff/14001/src/pkg/runtime/time.goc#ne...
src/pkg/runtime/time.goc:100: // when must never be negative otherwise timerproc
will overflow
On 2013/05/21 00:34:34, r wrote:
> s/negative/&;/

Done.

https://codereview.appspot.com/9035047/diff/14001/src/pkg/runtime/time.goc#ne...
src/pkg/runtime/time.goc:101: // during it's delta calculation and never expire
other timers.
On 2013/05/21 00:34:34, r wrote:
> s/it's/its/

Done.

https://codereview.appspot.com/9035047/diff/14001/src/pkg/time/internal_test.go
File src/pkg/time/internal_test.go (right):

https://codereview.appspot.com/9035047/diff/14001/src/pkg/time/internal_test....
src/pkg/time/internal_test.go:38: const timeout = 5 * Millisecond
On 2013/05/21 00:38:54, r wrote:
> is this long enough for slow machines? i fear flakiness ahead.
> 
> i'd make the "timeout" very long - like a second, and the timer very fast,
like
> 1ms.

Done. I've done it a little differently.
Also, the comment about the deferred func
is updated.

[ality, 2013-06-05 00:42:14]

Ping.

I changed the test slightly since the last LGTM
so I'd like to get someone to look it over once
more before I submit.

Thanks,
  Anthony

[mikio, 2013-06-06 04:13:21]

LGTM

[r, 2013-06-06 13:00:17]

LGTM

[r, 2013-06-08 15:20:43]

LGTM
i'm sure this will flake out on our crappy builders, but let's see it happen
before we tune

[bradfitz, 2013-06-17 20:19:54]

R=r

[remyoudompheng, 2013-06-26 15:21:51]

ping

It seems this patch is approved.

[rsc, 2013-06-27 03:18:02]

sorry, just overwhelmed by all the runtime stuff. will review tomorrow.

[rsc, 2013-06-27 15:48:47]

LGTM

https://codereview.appspot.com/9035047/diff/35001/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

https://codereview.appspot.com/9035047/diff/35001/src/pkg/runtime/time.goc#ne...
src/pkg/runtime/time.goc:22: static void dumptimers(byte*);
make this int8* and drop the casts

[ality, 2013-07-20 07:45:39]

Sorry for the monthlong absence.

I'll submit this and see if the builders like it.
Fingers crossed.

  Anthony

https://codereview.appspot.com/9035047/diff/35001/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

https://codereview.appspot.com/9035047/diff/35001/src/pkg/runtime/time.goc#ne...
src/pkg/runtime/time.goc:22: static void dumptimers(byte*);
On 2013/06/27 15:48:48, rsc wrote:
> make this int8* and drop the casts

Done.

[dvyukov, 2013-08-10 10:42:17]

On 2013/07/20 07:45:39, ality wrote:
> Sorry for the monthlong absence.
> 
> I'll submit this and see if the builders like it.
> Fingers crossed.
> 
>   Anthony


ping

[mikio, 2013-08-21 09:55:06]

ping

[rsc, 2013-09-06 19:47:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=0a11e3e40738 ***

runtime: handle timer overflow in tsleep

Make sure we never pass a timer into timerproc with
a negative duration since it will cause other timers
to never expire.

Fixes issue 5321.

R=golang-dev, minux.ma, remyoudompheng, mikioh.mikioh, r, bradfitz, rsc, dvyukov
CC=golang-dev
https://codereview.appspot.com/9035047

Committer: Russ Cox <rsc@golang.org>