code review 202043: time: Sleep through interruptions

time: Sleep through interruptions

[cw, 2010-02-04 07:00:45]

Hello r, rsc, iant (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2010-02-04 07:23:12]

Thanks for fixing this.
Please make the description (hg change 202043)
a one-liner that stands alone in an hg log, like

time: Sleep through interruptions

http://codereview.appspot.com/202043/diff/1001/4
File src/pkg/time/sleep.go (right):

http://codereview.appspot.com/202043/diff/1001/4#newcode12
src/pkg/time/sleep.go:12: // Sleep pauses the current goroutine for at least ns
please undo gratuitous formatting change.
just delete the final sentence (about EINTR).
look at the diff to see what i mean.

http://codereview.appspot.com/202043/diff/1001/4#newcode18
src/pkg/time/sleep.go:18: left := ns
simpler:

t := Nanoseconds()
end := t + ns
for t < end {
    errno := syscall.Sleep(ns)
    if errno != 0 && errno != syscall.EINTR {
        return os.NewSyscallError("sleep", errno)
    }
    t = Nanoseconds()
}
return nil

please add a test that tries to sleep for 100 ms
but sends a SIGINTR after 50ms.

[cw, 2010-02-04 07:48:04]

On Thu, Feb 04, 2010 at 07:23:12AM +0000, rsc@golang.org wrote:

> t := Nanoseconds()
> end := t + ns
> for t < end {
>     errno := syscall.Sleep(ns)

Sleep(ns) isn't right, it means we always sleep for the total time not
the time remaining

it should be Sleep(end-t)

>     if errno != 0 && errno != syscall.EINTR {
>         return os.NewSyscallError("sleep", errno)
>     }
>     t = Nanoseconds()
> }
> return nil



> please add a test that tries to sleep for 100 ms
> but sends a SIGINTR after 50ms.

i did this using syscall.Kill ... but the interface on mcapple has
three arguments not two, so:

        const delay = int64(100e6)
        go func() {
                Sleep(delay / 2)
                syscall.Kill(os.Getpid(), syscall.SIGCHLD)
        }()

i doubt will work.

what's the simplest way to address that?

[cw, 2010-02-04 07:49:40]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-02-04 07:58:47]

looks pretty good

http://codereview.appspot.com/202043/diff/8/9
File src/pkg/time/sleep.go (right):

http://codereview.appspot.com/202043/diff/8/9#newcode25
src/pkg/time/sleep.go:25: return nil // silence compiler warning
drop comment; that's the return nil for success

http://codereview.appspot.com/202043/diff/8/10
File src/pkg/time/sleep_test.go (right):

http://codereview.appspot.com/202043/diff/8/10#newcode1
src/pkg/time/sleep_test.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
2010

http://codereview.appspot.com/202043/diff/8/10#newcode16
src/pkg/time/sleep_test.go:16: const delay = int64(500e6)
type is unnecessary

const delay = 500e6 // 500 ms

can it be more like 100ms?
500ms is a long time for a test to run

http://codereview.appspot.com/202043/diff/8/10#newcode23
src/pkg/time/sleep_test.go:23: duration := Nanoseconds() - start
t := Nanoseconds() - start

http://codereview.appspot.com/202043/diff/8/10#newcode24
src/pkg/time/sleep_test.go:24: if duration < 100e6 {
if t < delay {
    t.Errorf("Sleep(%d) slept for %d", delay, t)
}

[rsc, 2010-02-04 08:06:53]

> what's the simplest way to address that?

bah.  i hate unix.  i'll fix the syscall package
to make Kill 2 arguments everywhere.

russ

[cw, 2010-02-04 08:12:08]

On Thu, Feb 04, 2010 at 12:06:48AM -0800, Russ Cox wrote:

> bah.  i hate unix.  i'll fix the syscall package
> to make Kill 2 arguments everywhere.

i could just ForkExec /bin/true ... in fact i coded that up already,
though for some reason it's not working (might be something else)

[cw, 2010-02-04 08:15:34]

On Thu, Feb 04, 2010 at 07:58:47AM +0000, rsc@golang.org wrote:

> type is unnecessary
>
> const delay = 500e6 // 500 ms

it is

Sleep(delay/2) has a rounding error without it

> can it be more like 100ms?

yes

> 500ms is a long time for a test to run

sorry, that wasn't supposed to get submitted like that, i was making
the delay longer trying to test something

> http://codereview.appspot.com/202043/diff/8/10#newcode23
> src/pkg/time/sleep_test.go:23: duration := Nanoseconds() - start
> t := Nanoseconds() - start

t is already used:

func TestSleep(t *testing.T) {

> if t < delay {

yeah, i spotted that one as well.  thanks.

[rsc, 2010-02-04 09:55:35]

http://codereview.appspot.com/202043/diff/1013/18
File src/pkg/time/sleep_test.go (right):

http://codereview.appspot.com/202043/diff/1013/18#newcode26
src/pkg/time/sleep_test.go:26: t.Fatalf("Sleep slept for only %d nanoseconds
(expected %d)", duration, delay)
please describe code with code

t.Fatalf("Sleep(%d) slept for only %d ns", delay, duration)

[cw, 2010-02-04 09:59:08]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-02-04 21:09:04]

*** Submitted as http://code.google.com/p/go/source/detail?r=30bb6960300c ***

time: Sleep through interruptions

R=rsc
CC=golang-dev
http://codereview.appspot.com/202043

Committer: Russ Cox <rsc@golang.org>