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

Propagate signals in intermediate bash shells #1116

Merged
merged 4 commits into from Dec 5, 2019

Conversation

lox
Copy link
Contributor

@lox lox commented Nov 4, 2019

If you execute multiple commands, e.g:

steps:
  -label: "My commands" 
   commands:
     - ./process1
     - ./process2

then we join them together in a single bash shell as /bin/bash -e -c './process1\b./process2, which has the side effect of creating an intermediate shell in between the bootstrap process and your commands. By default, most shells won't automatically propagate signals to subprocesses, so when the job is cancelled the signal gets lost in this intermediate shell.

This could be manually fixed by adding a trap as the first command:

steps:
  -label: "My commands" 
   commands:
     - trap "kill -- \$\$" TERM QUIT INT
     - ./process1
     - ./process2

That ends up kind of noisy and for those not versed in shell voodoo, confusing.

This PR automatically adds this into non-script commands.

If you execute multiple commands, for instance [a, b, c] then we join
them together in a single bash shell. This has the side effect of
creating and intermediate bash shell where you might not expect it. This
adds a trap that propagates signals to the process group when it gets
them.

This means that cancel signals propagate correctly.
@lox lox force-pushed the add-trap-to-intermediate-bash-shells-in-bootstrap branch from 5310c4e to 3ace667 Compare November 4, 2019 02:23
@toolmantim
Copy link
Contributor

Noice fix! 👌🏼

Copy link
Contributor

@matthewd matthewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this -- if we're the ones who've decided to inject an extra shell into the process hierarchy, then it follows that we're obliged to ensure it behaves as transparently as possible within our process-management paradigm. 👍🏻

bootstrap/bootstrap.go Outdated Show resolved Hide resolved
@lox lox merged commit 88578a7 into master Dec 5, 2019
@lox
Copy link
Contributor Author

lox commented Jul 8, 2020

Past @lox, just as an FYI, I find trap command in all build output really confusing. The incantation of trap "kill -- \$\$" TERM QUIT INT could probably be hidden in non-debug mode.

@yob yob deleted the add-trap-to-intermediate-bash-shells-in-bootstrap branch August 10, 2020 12:07
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 this pull request may close these issues.

None yet

3 participants