os/signal: deadlock detection fails when os/signal is included

[cwedgwood]

go1.8, go1.9-rc2

linux amd64 & go playground

including os/signal breaks deadlock detection:

package main

import _ "os/signal"

func main() {
	c := make(chan int)
	c <- 1
}

or see https://play.golang.org/p/-KF7aAeIhS as it happens there as well

this hangs

without os/signal included we get the expected deadlock detection message

[ianlancetaylor]

This is hard to fix. A program that has any active signal.Notify channels can not be deadlocked, since a signal could arrive at any time. So it would have to be the case that we report a deadlock only when no channels remain in signal.Notify. That is difficult because os/signal starts up a helper goroutine to wait for signals to arrive. To fix this we would have to start that helper goroutine only when Notify is called, and shut it down when Stop is called on all active channels. I don't think it's worth it for such an obscure case.

[cwedgwood]

@ianlancetaylor understood, but as is - even if i call no functions explicitly (such as when the code is in dev/debug mode), the init functions are active here

this means to 'enable' deadlock detection i need a code-change where i remove all references to os/signal

i don't have a clean solution to propose here, i'm just highlighting a build/debug quirk that came up

[ianlancetaylor]

I would be willing to consider a CL that starts the goroutine only when Notify is called, not at initialization time. I haven't looked too closely but I think that would be a fairly simple change and an improvement over the current situation.

[gopherbot]

Change https://golang.org/cl/101036 mentions this issue: os/signal: lazily start signal watch loop only on Notify

[odeke-em]

I stumbled upon this issue as I was having dinner so I've mailed https://go-review.googlesource.com/#/c/go/+/101036. Thank you very much for the report @cwedgwood and @ianlancetaylor for the solution!

[cwedgwood]

resolved some time ago, closing

[odeke-em]

This issue hasn't yet been solved. CL fixing it will be merged soon.

[odeke-em]

I mailed CL https://go-review.googlesource.com/c/go/+/101036 for this change, PTAL.

[shawndx]

Hi,

Just curious to know is "ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)" in the associated testcase supposed to kill the background 'go run' after 5 seconds (if without the fix from @odeke-em , it gets stuck there with my dev. environment?

If reducing the time-out value, say to 100 millisecond, the background go program could be killed, what's behind the difference?

Thanks.

[odeke-em]

Too low of a deadline and it could become a flake during tests, too high of a deadline and it could stall building the Go tree and running other tests. 5seconds is a heuristic that I thought was a decently good enough time for go run to have successfully started. Makes sense? Thank you.

…

On Wed, Oct 9, 2019 at 7:19 AM Shawn ***@***.***> wrote: Hi, Just curious to know is "ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)" in the associated testcase supposed to kill the background 'go run' after 5 seconds (if without the fix from @odeke-em <https://github.com/odeke-em> , it gets stuck there with my dev. environment? If reducing the time-out value, say to 100 millisecond, the background go program could be killed, what's behind the difference? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21576?email_source=notifications&email_token=ABFL3V4H5PRNFWMLMLECDATQNW43NA5CNFSM4DYB346KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAXRJBA#issuecomment-539956356>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFL3V6KJTLCIQ7IB76BQ53QNW43NANCNFSM4DYB346A> .

[shawndx]

Thanks @odeke-em . Furthermore, without your change is the background 'go run' supposed to be killed after 5 seconds due to time-out?