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

Go bazelisk causes double-SIGINT in "bazel run" #307

Closed
philsc opened this issue Mar 15, 2022 · 0 comments · Fixed by #322
Closed

Go bazelisk causes double-SIGINT in "bazel run" #307

philsc opened this issue Mar 15, 2022 · 0 comments · Fixed by #322

Comments

@philsc
Copy link

philsc commented Mar 15, 2022

With this py_binary():

import time
import signal

running = True

def _sigint_handler_debounced(signal_number, frame):
    global running
    if not running:
        print("Ignoring superfluous SIGINT")
        return

    print("Stopping the program")
    running = False


signal.signal(signal.SIGINT, _sigint_handler_debounced)

while running:
    time.sleep(1)

print("Almost there. Waiting for potential superfluous SIGINTs")
time.sleep(1)

And then when I run it and hit CTRL-C once:

$ bazel run //common/python/double_sigint:foo
^CStopping the program
Ignoring superfluous SIGINT
Almost there. Waiting for potential superfluous SIGINTs

This appears to be caused by the signal forwarding in the Go version:

bazelisk/core/core.go

Lines 438 to 442 in 15abf0e

signal.Notify(c, os.Interrupt, syscall.SIGTERM)
go func() {
s := <-c
if runtime.GOOS != "windows" {
cmd.Process.Signal(s)

The Python version notably does not have this behaviour:

bazelisk/bazelisk.py

Lines 445 to 452 in 81f38ed

p = subprocess.Popen([cmd["exec"]] + cmd["args"], close_fds=os.name != "nt", env=cmd["env"])
while True:
try:
return p.wait()
except KeyboardInterrupt:
# Bazel will also get the signal and terminate.
# We should continue waiting until it does so.
pass

I'd be tempted to say that bazelisk should only forward SIGTERM and not forward SIGINT.
I.e. hitting CTRL-C on an interactive program should only generate a single SIGINT in the running program. But sending a SIGTERM to bazelisk (via child PID after fork/exec) should also trigger a SIGTERM in the running process.

What do the maintainers here think?

gibfahn added a commit to gibfahn/bazelisk that referenced this issue Jun 6, 2022
Since we handle os.Interrupt but no longer forward it to child
processes, this commit also removes platform-specific handling for
Windows, preferring to use child.Process.Kill() in all cases.

Fixes bazelbuild#307.
fweikert pushed a commit that referenced this issue Jun 20, 2022
Since we handle os.Interrupt but no longer forward it to child
processes, this commit also removes platform-specific handling for
Windows, preferring to use child.Process.Kill() in all cases.

Fixes #307.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant