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

os/signal: add func Ignored(sig Signal) bool #22497

Closed
adam-azarchs opened this issue Oct 30, 2017 · 21 comments
Closed

os/signal: add func Ignored(sig Signal) bool #22497

adam-azarchs opened this issue Oct 30, 2017 · 21 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Milestone

Comments

@adam-azarchs
Copy link
Contributor

Summary

Generally speaking, when a Go process receives a signal it will either be handled (by the runtime or by application code), ignored, or will terminate the process. There are several scenarios where it is useful to know which one will happen, and also what the original behavior was for the process (e.g. what signal.Reset(sig) will change it to).

On unix systems, the current disposition can be obtained by runtime.getsig and the original disposition is in runtime.fwdSig. Neither of these are exposed to the application.

Example use case

Determining whether SIGHUP is ignored (which will be the case if run through nohup) before registering a handler. The go runtime already does this, but there isn't a great way for application code to do it.

Proposed API

Exposing the function pointers returned by getsig would be a bad idea. Instead, the proposal is to add to the os/signal package a type

type Status int
const (
    Default = Disposition(iota)
    Ignored
    Notify
    Panic
    Terminate
)

and methods

func CurrentStatus(sig os.Signal) Status {
    // on unix, this information can be found by a combination of runtime.getsig,
    // runtime.handlingSig, and runtime.sigtable
}

func StartingStatus(sig os.Signal) Status {
    // on unix, this information can be found by a combination of runtime.fwdSig
    // and runtime. sigtable
}
@gopherbot gopherbot added this to the Proposal milestone Oct 30, 2017
@dsnet
Copy link
Member

dsnet commented Oct 30, 2017

\cc @bcmills

@dsnet dsnet changed the title proposal: API in os.signal to query current signal disposition proposal: os/signal: API to query current signal disposition Oct 30, 2017
@adam-azarchs
Copy link
Contributor Author

Worth noting: on Unix at least, asynchronous and real-time signals have a lot of subtleties in multi-threaded applications which are not captured in this proposal. I think most application developers don't care about those issues and just want to know what will happen when someone says kill -1 <pid>, and if they care about the subtle details at some point they should probably just be CGO-importing signal.h.

@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2017

The API that you propose is inherently racy. The os/signal package is already unpleasantly racy, but at the very least we should not make it worse.

The go runtime already does this

runtime.sigInstallGoHandler tells the runtime what signal handler was installed when the runtime initialized. It does not indicate what happened at program start and does not indicate the current disposition of the signal — other C packages may have registered or ignored handlers before or after the Go runtime.


It is only safe to replace C signal handlers while the program is known to be single-threaded: the POSIX signal API does not provide a compare-and-swap flag, so there is no way to replace a signal handler without potentially racing with other libraries doing the same, and it is not in general safe for any library to assume that it has exclusive control over the program's signal handlers (see #20400).

The call to sigInstallGoHandler from runtime.initsig should occur very early in the program. If it is currently happening after threads may be started, we should probably move it earlier. The calls from sigdisable and setProcessCPUProfiler are racy, and arguably we should remove them if we can. (It isn't obvious to me whether we can do so without breaking Go 1 compatibility, but at the very least we can warn against using them in non-trivial programs, or possibly deprecate certain functions or modes of usage.)

See also #20748 — another race involving signal disposition and registration — and my comments on change 42830.

If a library correctly avoids replacing handlers after threads have been started, any handlers it has registered should enable and disable themselves by setting atomic variables within the handler library, not by registering and unregistering the handler itself. (The state may be even more ambiguous once #19465 is addressed.)

So the proposed CurrentStatus function is impossible to write portably: at best, it could tell the user whether the Go runtime is currently handling the signal, without knowing what any existing or subsequent C handler may be doing.

The proposed StartingStatus function is similar: it can tell you whether the Go runtime initially registered its own handler, but it cannot tell you what any existing (or subsequent) C handler is doing. And if you want to know whether the Go runtime has registered a signal handler (or what handler was present before the Go runtime initialized), you can always call sigaction from C at the appropriate time and check for yourself.

@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2017

Honestly, I think we should discourage the use of signal.Reset and signal.Ignore entirely. They both rely on assumptions about the behavior of all other libraries in the process, and those assumptions do not scale.

In contrast, signal.Notify and signal.Stop can at least be fixed in theory: the runtime can go ahead and register handlers for HUP and INT before threads start, then drop the signals explicitly in the handler if they were previously SIG_IGN. (If we went that route, we would presumably need to fix up the signal masks in os/exec too.)

@adam-azarchs
Copy link
Contributor Author

There absolutely is a potential for races here, depending on implementation details. However that's neither here nor there for the main use cases. There are cases where an application might want to handle SIGHUP or SIGINT even if the program started out ignoring the signals. There are cases where handlers might behave differently depending on what the disposition started out as.

Perhaps the concerns about concerns about race conditions could be mitigated without loosing the main useful functionality by limiting the scope of the API to determining what the "default" behavior for a signal is before any application handlers were registered. That is, how does the go runtime treat the signal, and was it ignored when the runtime started up.

@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2017

There are cases where an application might want to handle SIGHUP or SIGINT even if the program started out ignoring the signals.

About the only thing you can't do with the os/signal API right now is make the Notify call conditional on whether the signal was already ignored. So, what's the concrete problem you're trying to address by doing that?

@adam-azarchs
Copy link
Contributor Author

Granted having Notify be a no-op if the signal is ignored would solve 95% of use cases I can imagine. However I can imagine an application which would wish to do something with SIGINT even in the "ignored" case, for example write something to a log, but exit iff the signal was not previously ignored. It's also useful to be able to inspect signal disposition when writing unit tests for code which is supposed to set up handlers. I could also imagine cases where it be useful to be able to programatically enumerate which signals are handled by the runtime (e.g. SIGILL) and should not be handled by user code.

@adam-azarchs
Copy link
Contributor Author

I'd also be worried about the compatibility implications of changing Notify to do nothing for ignored SIGHUP, in case someone was relying on being able to catch it. Giving the user the tools they need to decide for themselves seems safer in that regard.

@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2017

for example write something to a log, but exit iff the signal was not previously ignored.

If you know that your handler is the only one in the process, you can forward the signal yourself (modulo #19326):

	c := make(chan os.Signal, 1)
	signal.Notify(c, syscall.SIGINT)
	go func() {
		runtime.LockOSThread()
		for {
			sig := <-c
			signal.Stop(c)
			syscall.Tgkill(syscall.Getpid(), syscall.Gettid(), sig)
			signal.Notify(c, syscall.SIGINT)
		}
	}()

If you don't know that your handler is the only one in the process, trying to achieve that sort of behavior via the signal API isn't scalable anyway: how do you know that there isn't some other SIGINT handler in the process that either you will prevent from running (if you exit too early) or will prevent you from running (if it exits too early)?

You could imagine some entirely different API for forwardable signals in Go, but it's not obvious to me how to do that without producing an API that is prone to deadlocks, fails to interoperate with C handlers, and/or fails to interoperate with the existing os/signal API.

@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2017

It's also useful to be able to inspect signal disposition when writing unit tests for code which is supposed to set up handlers.

That one is easier: write your tests to execute the Go code as a subprocess using a known signal mask.

@ianlancetaylor
Copy link
Contributor

I don't see the point of CurrentStatus. Your program can keep track of this itself. If it can't, then using CurrentStatus will indeed by racy.

But I do see the point of something like StartingStatus. Right now if SIGHUP and SIGINT are ignored when a Go program starts, they will continue to be ignored. But calling Notify will cause them to not be ignored. It does seem desirable to have some way for a program to only call Notify for SIGHUP if SIGHUP is not currently ignored. Otherwise, the program's desire to have a SIGHUP handler will override the user's desire to use nohup.

@rsc
Copy link
Contributor

rsc commented Nov 6, 2017

What if we just add to os/signal

// Ignored reports whether sig is currently ignored.
func Ignored(sig Signal) bool

?

@adam-azarchs
Copy link
Contributor Author

I think it's true that it's reasonable to ask application code to keep track of the current status if it needs to, especially since it's potentially racy. But being able to ask whether it was ignored at runtime startup is useful and not racy.

@ianlancetaylor
Copy link
Contributor

@adam-azarchs Right, you can use @rsc's proposed Ignored function to ask exactly that, before calling Notify.

@rsc
Copy link
Contributor

rsc commented Nov 13, 2017

@adam-azarchs, does func Ignored (in #22497 (comment)) solve your problem?

@adam-azarchs
Copy link
Contributor Author

Yes, it does. My only concern about it is more around documentation of the various possible races, but it does solve the problem.

@rsc rsc changed the title proposal: os/signal: API to query current signal disposition os/signal: add func Ignored(sig Signal) bool Nov 20, 2017
@rsc rsc modified the milestones: Proposal, Go1.11 Nov 20, 2017
@rsc
Copy link
Contributor

rsc commented Nov 20, 2017

Proposal accepted then, scoped to the func Ignored described above. CLs welcome for Go 1.11.

@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Nov 20, 2017
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/98835 mentions this issue: os/signal: add func Ignored(sig Signal) bool

@ysmolski
Copy link
Member

ysmolski commented Mar 7, 2018

If anyone want to pick this up, that CL above was abandoned.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/108376 mentions this issue: os/signal: add func Ignored(sig Signal) bool

@adam-azarchs
Copy link
Contributor Author

Finally got around to getting my employer to sign the CLA. I've taken a crack at resolving this, at least on unix systems. The state of signal functionality in general on windows/nacl/plan9 seems to leave something to be desired.

@golang golang locked and limited conversation to collaborators Apr 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

8 participants