code review 47540043: runtime: co-exist with NPTL's pthread_cancel().

runtime: co-exist with NPTL's pthread_cancel.

NPTL uses SIGRTMIN (signal 32) to effect thread cancellation.
Go's runtime replaces NPTL's signal handler with its own, and
ends up aborting if a C library that ends up calling
pthread_cancel is used.

This patch prevents runtime from replacing NPTL's handler.

Fixes issue 6997.

[sqweek, 2014-01-07 18:58:34]

Hello golang-codereviews@googlegroups.com,

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

[iant, 2014-01-07 19:39:00]

Can you look into adding a test, perhaps in misc/cgo/test?

[dvyukov, 2014-01-08 06:45:25]

Doesn't this break existing Go programs that use SIGRTMIN?

[sqweek, 2014-01-08 08:17:20]

Yes I would expect so, as the runtime is no longer handling SIGRTMIN.

Unfortunately if the go runtime overwrites pthreads' signal handler to handle
SIGRTMIN, then it cannot interoperate with libraries that call pthread_cancel.

I can see three other alternatives to achieve the goal of pthreads coexistance:

1) provide a way for a go program to control the signals the runtime handles
(technically I guess this is already possible by having the program call a cgo
function that registers its own signal handler, overwriting the runtime's).

2) detect whether the pthreads signal handler is registered. if not, register
the runtime's signal handler as normal, but if so leave the pthreads signal
handler in place. This will break existing Go programs that use SIGRTMIN *and*
link with pthreads, but they are already accidents waiting to happen.

3) have the runtime signal handler remember the previous signal handler and call
through to it for signals received on a non-go thread. Not sure if this is
feasible in the face of sigaltstack() and friends.


PS. I'm working on a test case, and noticed the failure mode is different in the
more recent codebase. In go 1.1.2 the runtime aborted with message "signal
received on thread not created by Go: signal 32". A more recent runtime doesn't
give up in this scenario but with the signal not delivered to pthreads, the
thread we try to cancel ends up blocked in __libc_disable_asynccancel(),
spinning on a futex() call that keeps returning EAGAIN.

[iant, 2014-01-08 14:20:18]

On Tue, Jan 7, 2014 at 10:45 PM,  <dvyukov@google.com> wrote:
>
> Doesn't this break existing Go programs that use SIGRTMIN?

Programs that link against -lpthread and use SIGRTMIN (or SIGRTMIN+1)
are already broken.

We could only change the behaviour when using cgo, but I'm not sure
whether it's worth it.  I would be slightly more concerned if the
syscall package provided a SIGRTMIN constant, but it doesn't.  Any Go
program that uses SIGRTMIN must be doing it based only on the signal
number.  That seems an unlikely way to program.  And we are already
treating SIGRTMIN+1 specially.

Ian

[sqweek, 2014-01-08 17:08:45]

I've added the test case to this CL in patch set 6.

As I mentioned I can't reproduce the runtime exiting behaviour with the current
codebase, so I'm not 100% sure the test captures that failure mode. But I
presume it would result in cgo_test exiting with a non-zero status and be
interpreted as a test failure.

[iant, 2014-01-08 20:40:05]

https://codereview.appspot.com/47540043/diff/70001/misc/cgo/test/issue6997_li...
File misc/cgo/test/issue6997_linux.c (right):

https://codereview.appspot.com/47540043/diff/70001/misc/cgo/test/issue6997_li...
misc/cgo/test/issue6997_linux.c:1: // Copyright 2013 The Go Authors.  All rights
reserved.
s/2013/2014/

https://codereview.appspot.com/47540043/diff/70001/misc/cgo/test/issue6997_li...
File misc/cgo/test/issue6997_linux.go (right):

https://codereview.appspot.com/47540043/diff/70001/misc/cgo/test/issue6997_li...
misc/cgo/test/issue6997_linux.go:1: // Copyright 2013 The Go Authors.  All
rights reserved.
s/2013/2014/

https://codereview.appspot.com/47540043/diff/70001/misc/cgo/test/issue6997_li...
misc/cgo/test/issue6997_linux.go:7: // See
http://code.google.com/p/go/issuens/detail?id=6997
Write the link as http://golang.org/issue/6997

https://codereview.appspot.com/47540043/diff/70001/misc/cgo/test/issue6997_li...
misc/cgo/test/issue6997_linux.go:11: #cgo LDFLAGS: -lpthread
Since this is Linux specific, do this as

#cgo CFLAGS: -pthread
#cgo LDFLAGS: -pthread

[iant, 2014-01-08 21:47:12]

Also s/()// in the CL description, we don't usually that in Go comments.

[sqweek, 2014-01-09 10:17:47]

Done, and submitted individual contributer license agreement.

s/2013/2014/ needs applying here aswell: http://golang.org/doc/contribute.html

[iant, 2014-01-09 14:53:53]

On 2014/01/09 10:17:47, sqweek wrote:
> Done, and submitted individual contributer license agreement.

Thanks.  Unfortunately Google does require us to get people's real names for the
license agreement.  You've signed your name as "sqweek;" is that your real name?

[sqweek, 2014-01-09 15:44:32]

On 2014/01/09 14:53:53, iant wrote:
> You've signed your name as "sqweek;" is that your real name?

As real as any other, but I suppose I shouldn't be signing legal documents with
it. Sorry.

I've resubmitted the form using my legal name - do I need to modify this CL
aswell? I changed my name in .hg/hgrc, but I'm not sure what codereview command
to run (if any).

[iant, 2014-01-09 17:33:47]

LGTM

On 2014/01/09 15:44:32, sqweek wrote:
> 
> I've resubmitted the form using my legal name - do I need to modify this CL
> aswell? I changed my name in .hg/hgrc, but I'm not sure what codereview
command
> to run (if any).

No need to modify the CL, thanks.

Thanks for your patience working through this CL.

Ian

[iant, 2014-01-09 17:34:09]

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

runtime: co-exist with NPTL's pthread_cancel.

NPTL uses SIGRTMIN (signal 32) to effect thread cancellation.
Go's runtime replaces NPTL's signal handler with its own, and
ends up aborting if a C library that ends up calling
pthread_cancel is used.

This patch prevents runtime from replacing NPTL's handler.

Fixes issue 6997.

R=golang-codereviews, iant, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/47540043

Committer: Ian Lance Taylor <iant@golang.org>

[sqweek, 2014-01-10 01:44:40]

On 2014/01/09 17:33:47, iant wrote:
> Thanks for your patience working through this CL.

My pleasure. Thanks for the guidance, Ian and Dmitriy!

[tsuna, 2014-10-31 05:45:04]

Sorry to resurrect this review, I figured it was better to comment here than
send an email out of the blue to golang-dev.

https://codereview.appspot.com/47540043/diff/90001/misc/cgo/test/issue6997_li...
File misc/cgo/test/issue6997_linux.go (right):

https://codereview.appspot.com/47540043/diff/90001/misc/cgo/test/issue6997_li...
misc/cgo/test/issue6997_linux.go:37: case <-time.After(5 * time.Second):
Was there any particular reason for picking 5 seconds as a timeout?  It seems
that our continuous integration system at Arista is tripping on this timeout due
to some of our build servers being under heavy load from running multiple builds
concurrently.

5 seconds seems a bit aggressive.  Would you consider 30 seconds acceptable?

[iant, 2014-10-31 16:40:22]

On 2014/10/31 05:45:04, tsuna wrote:
> Sorry to resurrect this review, I figured it was better to comment here than
> send an email out of the blue to golang-dev.

For something this old commenting on golang-dev is probably better.


>
https://codereview.appspot.com/47540043/diff/90001/misc/cgo/test/issue6997_li...
> File misc/cgo/test/issue6997_linux.go (right):
> 
>
https://codereview.appspot.com/47540043/diff/90001/misc/cgo/test/issue6997_li...
> misc/cgo/test/issue6997_linux.go:37: case <-time.After(5 * time.Second):
> Was there any particular reason for picking 5 seconds as a timeout?  It seems
> that our continuous integration system at Arista is tripping on this timeout
due
> to some of our build servers being under heavy load from running multiple
builds
> concurrently.
> 
> 5 seconds seems a bit aggressive.  Would you consider 30 seconds acceptable?

Sure, send a CL.

[tsuna, 2014-10-31 17:59:52]

On 2014/10/31 16:40:22, iant wrote:
> Sure, send a CL.

Done: https://codereview.appspot.com/170850043/

Thanks.

[minux, 2014-10-31 18:36:15]

On Oct 31, 2014 12:35 PM, <tsunanet@gmail.com> wrote:
>
https://codereview.appspot.com/47540043/diff/90001/misc/cgo/test/issue6997_li...
> misc/cgo/test/issue6997_linux.go:37: case <-time.After(5 * time.Second):
> Was there any particular reason for picking 5 seconds as a timeout?  It
> seems that our continuous integration system at Arista is tripping on
> this timeout due to some of our build servers being under heavy load
> from running multiple builds concurrently.
>
> 5 seconds seems a bit aggressive.  Would you consider 30 seconds
> acceptable?
i'm curious, are you using patched Go? why are you doing misc/cgo/test in
your CI?

[tsuna, 2014-10-31 18:37:22]

On 2014/10/31 18:36:15, minux wrote:
> i'm curious, are you using patched Go? why are you doing misc/cgo/test in
> your CI?

Nope, just building the release tarball into an RPM.