Only trap INT signal and set to DEFAULT #6223

shayonj · 2017-12-23T18:52:12Z

What was the end-user problem that led to this PR?

The problem was commands like nohup bundler exec {program} wouldn't work as intended. For example, if a HUP signal were to be sent to the process running the bundle exec .., it should in theory not terminate. Because, nohup would IGNORE that signal. But, that is not what the case is case is currently.

What was your diagnosis of the problem?

My diagnosis was, if a process/bundler execution already had a SIGNAL set to it, example HUP, then performing bundle exec {program}, would mean that bundler resets any prior SIGNALs on that process and sets them to DEFAULT.

What is your fix for the problem, implemented in this PR?

My fix to the problem is to only trap SIGNALs that we think should be set to DEFAULT, in this case, INT.

Why did you choose this fix out of the possible options?

I chose this fix because its lot less aggressive than setting every signal to DEFAULT, and gives us the work with a smaller set of SIGNALs. It also felt cleaner than having to trap a signal first and then restore to its predecessor value.

This is a dump that shows the before and after signals, when nohup bundle exec {program } gets run.

SIGEXIT: Before Handler: (), Current Handler: (DEFAULT)
SIGHUP: Before Handler: (IGNORE), Current Handler: (DEFAULT)
SIGINT: Before Handler: (#<Proc:0x00007f8e100534f8@/Users/<>/.rvm/gems/ruby-2.4.2/gems/bundler-1.16.0/exe/bundle:5>), Current Handler: (DEFAULT)
SIGQUIT: Before Handler: (DEFAULT), Current Handler: (DEFAULT)
SIGTRAP: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGABRT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGIOT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGEMT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGSYS: Before Handler: (), Current Handler: (DEFAULT)
SIGPIPE: Before Handler: (), Current Handler: (DEFAULT)
SIGALRM: Before Handler: (DEFAULT), Current Handler: (DEFAULT)
SIGTERM: Before Handler: (DEFAULT), Current Handler: (DEFAULT)
SIGURG: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGTSTP: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGCONT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGCHLD: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGCLD: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGTTIN: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGTTOU: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGIO: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGXCPU: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGXFSZ: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGPROF: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGWINCH: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)
SIGUSR1: Before Handler: (DEFAULT), Current Handler: (DEFAULT)
SIGUSR2: Before Handler: (DEFAULT), Current Handler: (DEFAULT)
SIGINFO: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT)

From this, we can see only INT is being trapped by bundler

SIGINT: Before Handler: (#<Proc:0x00007f8e100534f8@/Users/<>/.rvm/gems/ruby-2.4.2/gems/bundler-1.16.0/exe/bundle:5>), Current Handler: (DEFAULT)

hence, the only one being restored back to DEFAULT

Issue: #6150

segiddins · 2017-12-24T02:13:20Z

lib/bundler/cli/exec.rb

@@ -6,7 +6,7 @@ module Bundler
  class CLI::Exec
    attr_reader :options, :args, :cmd

-    RESERVED_SIGNALS = %w[SEGV BUS ILL FPE VTALRM KILL STOP].freeze
+    RESTORE_SIGNALS_TO_DEFAULT = %w[INT].freeze


TRAPPED_SIGNALS

segiddins · 2017-12-24T02:13:38Z

Looks good, can you please add a test for this change? Thanks!

shayonj · 2017-12-24T02:43:10Z

@segiddins There is a spec specifically for INT

https://github.com/bundler/bundler/blob/d5856a036642ae1f255c3bd301cd6937d1ed87bd/spec/commands/exec_spec.rb#L715

Which otherwise (without trapping the signal) fails. Do you think we should still introduce a new spec?

segiddins · 2017-12-24T08:16:42Z

Yes, we should add a spec that other signals aren't changed back to the default if they're set to ignore (i.e. testing the nohup case that led to this issue)

shayonj · 2017-12-24T20:26:18Z

Sounds good! I added spec for all signals that we could test for.

olleolleolle · 2018-01-08T17:50:08Z

@segiddins Has your feedback been met with that test?

oneamtu · 2018-01-11T22:54:30Z

As the end-user who signaled 🚦 this problem, thanks for your patch!

segiddins · 2018-01-13T05:03:50Z

spec/support/helpers.rb

    def sys_exec(cmd)
      command_execution = CommandExecution.new(cmd.to_s, Dir.pwd)

      Open3.popen3(cmd.to_s) do |stdin, stdout, stderr, wait_thr|
+        test_signals.each do |n|


this probably shouldn't be executed on every call to sys_exec?

I didn't see any concerns, but fair, moved to bundle, as its more clear now.

No, I mean, shouldn't this just be done for the single test case?

UPDATE: I figured, it'd make sense to have a consistent scenario of signal handling in all executions of bundle exec, such that a change on either side would result in a failing spec. But, perhaps, its a little too harsh.

I updated so that signal trapping is now specific to the spec itself.

segiddins · 2018-01-15T22:48:49Z

Perfect, thanks! I'm going to make a couple of tiny changes, but this is basically good to go 👍

Before, we were trapping every non reserved signal and setting it to DEFAULT. This meant, if there was a prior signal set on a process, we were resetting it with DEFAULT

olleolleolle · 2018-02-01T09:57:58Z

@segiddins Thanks for making those test changes!

Is this PR good to go now?

segiddins · 2018-02-03T19:31:16Z

@bundlerbot r+

bundlerbot · 2018-02-03T19:31:16Z

📌 Commit 3f304f7 has been approved by segiddins

bundlerbot · 2018-02-03T19:31:23Z

⌛ Testing commit 3f304f7 with merge 5cf764d...

Only trap INT signal and set to DEFAULT ### What was the end-user problem that led to this PR? The problem was commands like `nohup bundler exec {program}` wouldn't work as intended. For example, if a `HUP` signal were to be sent to the process running the `bundle exec ..`, it should in theory not terminate. Because, `nohup` would `IGNORE` that signal. But, that is not what the case is case is currently. ### What was your diagnosis of the problem? My diagnosis was, if a process/bundler execution already had a `SIGNAL` set to it, example `HUP`, then performing `bundle exec {program}`, would mean that bundler resets any prior `SIGNAL`s on that process and sets them to `DEFAULT`. ### What is your fix for the problem, implemented in this PR? My fix to the problem is to only trap `SIGNAL`s that we think should be set to `DEFAULT`, in this case, `INT`. ### Why did you choose this fix out of the possible options? I chose this fix because its lot less aggressive than setting every signal to `DEFAULT`, and gives us the work with a smaller set of `SIGNAL`s. It also felt cleaner than having to trap a signal first and then restore to its predecessor value. ---- This is a dump that shows the before and after signals, when `nohup bundle exec {program }` gets run. ``` SIGEXIT: Before Handler: (), Current Handler: (DEFAULT) SIGHUP: Before Handler: (IGNORE), Current Handler: (DEFAULT) SIGINT: Before Handler: (#<Proc:0x00007f8e100534f8@/Users/<>/.rvm/gems/ruby-2.4.2/gems/bundler-1.16.0/exe/bundle:5>), Current Handler: (DEFAULT) SIGQUIT: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGTRAP: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGABRT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGIOT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGEMT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGSYS: Before Handler: (), Current Handler: (DEFAULT) SIGPIPE: Before Handler: (), Current Handler: (DEFAULT) SIGALRM: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGTERM: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGURG: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGTSTP: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGCONT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGCHLD: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGCLD: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGTTIN: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGTTOU: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGIO: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGXCPU: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGXFSZ: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGPROF: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGWINCH: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGUSR1: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGUSR2: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGINFO: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) ``` From this, we can see only `INT` is being trapped by bundler ``` SIGINT: Before Handler: (#<Proc:0x00007f8e100534f8@/Users/<>/.rvm/gems/ruby-2.4.2/gems/bundler-1.16.0/exe/bundle:5>), Current Handler: (DEFAULT) ``` hence, the only one being restored back to `DEFAULT` ---- Issue: #6150

bundlerbot · 2018-02-03T19:58:04Z

☀️ Test successful - status-travis
Approved by: segiddins
Pushing 5cf764d to master...

Only trap INT signal and set to DEFAULT ### What was the end-user problem that led to this PR? The problem was commands like `nohup bundler exec {program}` wouldn't work as intended. For example, if a `HUP` signal were to be sent to the process running the `bundle exec ..`, it should in theory not terminate. Because, `nohup` would `IGNORE` that signal. But, that is not what the case is case is currently. ### What was your diagnosis of the problem? My diagnosis was, if a process/bundler execution already had a `SIGNAL` set to it, example `HUP`, then performing `bundle exec {program}`, would mean that bundler resets any prior `SIGNAL`s on that process and sets them to `DEFAULT`. ### What is your fix for the problem, implemented in this PR? My fix to the problem is to only trap `SIGNAL`s that we think should be set to `DEFAULT`, in this case, `INT`. ### Why did you choose this fix out of the possible options? I chose this fix because its lot less aggressive than setting every signal to `DEFAULT`, and gives us the work with a smaller set of `SIGNAL`s. It also felt cleaner than having to trap a signal first and then restore to its predecessor value. ---- This is a dump that shows the before and after signals, when `nohup bundle exec {program }` gets run. ``` SIGEXIT: Before Handler: (), Current Handler: (DEFAULT) SIGHUP: Before Handler: (IGNORE), Current Handler: (DEFAULT) SIGINT: Before Handler: (#<Proc:0x00007f8e100534f8@/Users/<>/.rvm/gems/ruby-2.4.2/gems/bundler-1.16.0/exe/bundle:5>), Current Handler: (DEFAULT) SIGQUIT: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGTRAP: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGABRT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGIOT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGEMT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGSYS: Before Handler: (), Current Handler: (DEFAULT) SIGPIPE: Before Handler: (), Current Handler: (DEFAULT) SIGALRM: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGTERM: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGURG: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGTSTP: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGCONT: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGCHLD: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGCLD: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGTTIN: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGTTOU: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGIO: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGXCPU: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGXFSZ: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGPROF: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGWINCH: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) SIGUSR1: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGUSR2: Before Handler: (DEFAULT), Current Handler: (DEFAULT) SIGINFO: Before Handler: (SYSTEM_DEFAULT), Current Handler: (DEFAULT) ``` From this, we can see only `INT` is being trapped by bundler ``` SIGINT: Before Handler: (#<Proc:0x00007f8e100534f8@/Users/<>/.rvm/gems/ruby-2.4.2/gems/bundler-1.16.0/exe/bundle:5>), Current Handler: (DEFAULT) ``` hence, the only one being restored back to `DEFAULT` ---- Issue: #6150 (cherry picked from commit 5cf764d)

shayonj force-pushed the s/hup-fix branch 2 times, most recently from 8425a49 to 63fb259 Compare December 23, 2017 18:59

segiddins reviewed Dec 24, 2017

View reviewed changes

shayonj force-pushed the s/hup-fix branch from 63fb259 to 854e177 Compare December 24, 2017 02:35

shayonj force-pushed the s/hup-fix branch from be30dd3 to 14d2b3b Compare December 24, 2017 20:25

shayonj force-pushed the s/hup-fix branch 6 times, most recently from 27382f2 to 6a672b6 Compare December 25, 2017 01:49

segiddins reviewed Jan 13, 2018

View reviewed changes

shayonj force-pushed the s/hup-fix branch 3 times, most recently from 92e1002 to 8e6f572 Compare January 13, 2018 18:00

Shayon Mukherjee and others added 3 commits January 15, 2018 15:56

Only trap INT signal and set to DEFAULT

cc38f84

Before, we were trapping every non reserved signal and setting it to DEFAULT. This meant, if there was a prior signal set on a process, we were resetting it with DEFAULT

Add spec to make sure unexpected signals are not restored to DEFAULT

05f214a

Clean up signal handler test

3f304f7

segiddins force-pushed the s/hup-fix branch from 8e6f572 to 3f304f7 Compare January 16, 2018 02:32

bundlerbot merged commit 3f304f7 into rubygems:master Feb 3, 2018

colby-swandale added this to the 1.16.2 milestone Feb 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only trap INT signal and set to DEFAULT #6223

Only trap INT signal and set to DEFAULT #6223

shayonj commented Dec 23, 2017 •

edited

Loading

segiddins Dec 24, 2017

segiddins commented Dec 24, 2017

shayonj commented Dec 24, 2017

segiddins commented Dec 24, 2017

shayonj commented Dec 24, 2017

olleolleolle commented Jan 8, 2018

oneamtu commented Jan 11, 2018

segiddins Jan 13, 2018

shayonj Jan 13, 2018 •

edited

Loading

segiddins Jan 13, 2018

shayonj Jan 13, 2018 •

edited

Loading

shayonj Jan 13, 2018

segiddins commented Jan 15, 2018

olleolleolle commented Feb 1, 2018

segiddins commented Feb 3, 2018

bundlerbot commented Feb 3, 2018

bundlerbot commented Feb 3, 2018

bundlerbot commented Feb 3, 2018

Only trap INT signal and set to DEFAULT #6223

Only trap INT signal and set to DEFAULT #6223

Conversation

shayonj commented Dec 23, 2017 • edited Loading

What was the end-user problem that led to this PR?

What was your diagnosis of the problem?

What is your fix for the problem, implemented in this PR?

Why did you choose this fix out of the possible options?

segiddins Dec 24, 2017

Choose a reason for hiding this comment

segiddins commented Dec 24, 2017

shayonj commented Dec 24, 2017

segiddins commented Dec 24, 2017

shayonj commented Dec 24, 2017

olleolleolle commented Jan 8, 2018

oneamtu commented Jan 11, 2018

segiddins Jan 13, 2018

Choose a reason for hiding this comment

shayonj Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

segiddins Jan 13, 2018

Choose a reason for hiding this comment

shayonj Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

shayonj Jan 13, 2018

Choose a reason for hiding this comment

segiddins commented Jan 15, 2018

olleolleolle commented Feb 1, 2018

segiddins commented Feb 3, 2018

bundlerbot commented Feb 3, 2018

bundlerbot commented Feb 3, 2018

bundlerbot commented Feb 3, 2018

shayonj commented Dec 23, 2017 •

edited

Loading

shayonj Jan 13, 2018 •

edited

Loading

shayonj Jan 13, 2018 •

edited

Loading