code review 6250072: time: fix deadlock in Sleep(0)

time: fix deadlock in Sleep(0)
See time/sleep_test.go for repro.

[dvyukov, 2012-05-29 17:51:51]

Hello golang-dev@googlegroups.com (cc: patrick.allen.higgins@gmail.com),

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

[r, 2012-05-29 17:56:04]

LGTM

http://codereview.appspot.com/6250072/diff/3003/src/pkg/time/sleep_test.go
File src/pkg/time/sleep_test.go (right):

http://codereview.appspot.com/6250072/diff/3003/src/pkg/time/sleep_test.go#ne...
src/pkg/time/sleep_test.go:227: func TestSleepZero(t *testing.T) {
obscure enough to deserve a comment
// Test that Sleep(0) does not deadlock.

[dvyukov, 2012-05-29 17:58:36]

http://codereview.appspot.com/6250072/diff/3003/src/pkg/time/sleep_test.go
File src/pkg/time/sleep_test.go (right):

http://codereview.appspot.com/6250072/diff/3003/src/pkg/time/sleep_test.go#ne...
src/pkg/time/sleep_test.go:227: func TestSleepZero(t *testing.T) {
On 2012/05/29 17:56:04, r wrote:
> obscure enough to deserve a comment
> // Test that Sleep(0) does not deadlock.

Done.

[rsc, 2012-05-29 17:58:50]

I don't understand why the test is so complex.  It looks like

func TestSleepZero(t *testing.t) {
   time.Sleep(0) // used to hang
}

would suffice.

[dvyukov, 2012-05-29 18:02:11]

On Tue, May 29, 2012 at 9:58 PM, Russ Cox <rsc@golang.org> wrote:

> I don't understand why the test is so complex.  It looks like
>
> func TestSleepZero(t *testing.t) {
>   time.Sleep(0) // used to hang
> }
>
> would suffice.
>

If GC is not pending, and this goroutine will block on a chan or something
like that, then status/waitreason will be overwritten and then restored
back to Grunnable (by whoever sends to the chan). So the problem arises iff
the goroutine calls into sched due to pending GC.

[dvyukov, 2012-05-29 18:03:33]

On Tue, May 29, 2012 at 10:02 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Tue, May 29, 2012 at 9:58 PM, Russ Cox <rsc@golang.org> wrote:
>
>> I don't understand why the test is so complex.  It looks like
>>
>> func TestSleepZero(t *testing.t) {
>>   time.Sleep(0) // used to hang
>> }
>>
>> would suffice.
>>
>
> If GC is not pending, and this goroutine will block on a chan or something
> like that, then status/waitreason will be overwritten and then restored
> back to Grunnable (by whoever sends to the chan). So the problem arises iff
> the goroutine calls into sched due to pending GC...
>
>
... while it has incorrect status=Gwaiting (if it blocks on chan/mutex,
then status=Gwaiting is *correct*).

[rsc, 2012-05-29 18:03:42]

Please add that comment to the test.

[dvyukov, 2012-05-29 18:14:14]

On 2012/05/29 18:03:42, rsc wrote:
> Please add that comment to the test.

Done. PTAL.

[rsc, 2012-05-29 18:26:55]

LGTM

Thanks for the explanation.

http://codereview.appspot.com/6250072/diff/4006/src/pkg/time/sleep_test.go
File src/pkg/time/sleep_test.go (right):

http://codereview.appspot.com/6250072/diff/4006/src/pkg/time/sleep_test.go#ne...
src/pkg/time/sleep_test.go:229: // Sleep(0) sets G's status to Gwaiting, but
then immidiately returns leaving the status.
s/immid/immed/

[dvyukov, 2012-05-29 18:31:11]

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

time: fix deadlock in Sleep(0)
See time/sleep_test.go for repro.

R=golang-dev, r, rsc
CC=golang-dev, patrick.allen.higgins
http://codereview.appspot.com/6250072