Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime/cgo: test signal from foreign thread before cgo call #10268

Open
crawshaw opened this issue Mar 27, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@crawshaw
Copy link
Contributor

commented Mar 27, 2015

A C++ program can trigger the Go signal handler from a global constructor before any cgo calls are made. As badsignal uses cgocallback, this requires an extra M be initialized. This was done for #10207, as it appears to sometimes happen with the os/signal tests on darwin/arm.

It needs a robust test.

There also needs to be some decision made about how this interacts with the deadlock detector. (Perhaps another issue.)

/cc @dvyukov

@crawshaw crawshaw self-assigned this Mar 27, 2015

@crawshaw crawshaw added this to the Go1.5 milestone Mar 27, 2015

@dvyukov

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

Does the signal happen in a global ctor? Or in a thread created by a global ctor?
I would expect that it is the latter. Because if it happens in a global ctor (in the main thread), then if a signal is delivered before Go runtime is initialized, then it also has not setup own signal handlers, thus badsignal won't be invoked; if it happens after runtime initialization, then the main thread also has a valid g/m, thus badsignal won't be invoked as well.

@crawshaw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2015

I assume in a foreign thread created by a global ctor. But I'm diagnosing from afar without code.

@crawshaw crawshaw changed the title runtime/cgo: test signal from global constructor runtime/cgo: test signal from foreign thread before cgo call Mar 27, 2015

@dvyukov

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

Maybe we want to split notion of extram's and disabled deadlock detection then? E.g. allow to have spare extram's but don't disable deadlock detection? And then create the first extram in schedinit, but disable deadlock detection on the first cgocall.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2015

It seems to me that we should disable deadlock detection if a program has any functions exported by cgo.

@minux

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2015

Yes, sigsend does nothing if sig.inuse is false, so one quick way to avoid many problems is for badsignal to do nothing in that case. It's not a real solution, though.

@dvyukov

This comment has been minimized.

Copy link
Member

commented Mar 28, 2015

@minux, it all sounds good, except for the very last part.
Signals are generally delivered to a random thread. Consider that an app registers a handler for SIGUSR1, and a user sends it from a console to update config or something. Now, once in a while the app just crashes (when the signal got delivered to that external thread) instead of processing the signal.

But I am curious whether the rest of what you proposed will fix the original problem or not.

@minux

This comment has been minimized.

Copy link
Member

commented Mar 28, 2015

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

We fixed #10139, which I think was important for Go 1.5. I don't believe this one is as important. It can likely wait until Go 1.6, although it's subtle enough it should be done early in the cycle.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

Rereading this thread, I'm completely lost. Can someone give a clear statement of the problem? (What did you do? What happened? What did you expect instead?)

If not, let's close this bug.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

Not to speak for @crawshaw, but I think this issue is really just about adding a test. As far as I can see the current code works.

See the lengthy discussion at the end of https://golang.org/cl/7978 .

@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Go1.6Early Nov 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.