bundle exec signal trapping regression from v1.11.2 to v1.12.0

[terlar]

There seems to be a regression between bundler version 1.11.2 and 1.12.0 (also the latest version).

Easy way to reproduce:

Gemfile:

source 'https://rubygems.org'

ruby '2.3.0'

gem 'puma'

config.ru:

run -> env { [200, { 'Content-Type' => 'text/plain' }, ['pong']] }

Working scenario:

$ gem install bundler -v 1.11.2
$ bundle
$ bundle exec puma
Puma starting in single mode...
* Version 3.4.0 (ruby 2.3.0-p0), codename: Owl Bowl Brawl
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://0.0.0.0:9292
Use Ctrl-C to stop
^C- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2016-05-12 11:27:51 +0200 ===
- Goodbye!
$ echo $?
0

Not working scenario:

$ gem install bundler -v 1.12.0
$ bundle
$ bundle exec puma
Puma starting in single mode...
* Version 3.4.0 (ruby 2.3.0-p0), codename: Owl Bowl Brawl
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://0.0.0.0:9292
Use Ctrl-C to stop
^C$ echo $?
1

[segiddins]

Can you verify this is still happening with 1.12.3?

[terlar]

Yes, it is still happening with 1.12.3. I just wanted to point out that the regression came with 1.12 series. I haven't bothered trying the rc and pre releases though.

$ bundle --version
Bundler version 1.12.3
$ bundle exec puma
Puma starting in single mode...
* Version 3.4.0 (ruby 2.3.0-p0), codename: Owl Bowl Brawl
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://0.0.0.0:9292
Use Ctrl-C to stop
^C$ echo $?
1

[terlar]

So walking through the released version I have found that:

1.12.0.pre.1 is working while 1.12.0.pre.2 is not. So it is a change between these versions that introduced it.

v1.12.0.pre.1...v1.12.0.pre.2

[agis]

Working on this. I'll have a proposed fix soon.

[segiddins]

This is due to the new use of load inside bundle exec.

[agis]

Yep. I did some digging and I initially thought that it's because execve(2) resets any defined signal traps on the new process, while Kernel.load does not. So when doing bundle exec puma, Bundler's trap runs instead of Puma's and exits prematurely.

Doing a trap("INT", "DEFAULT") just before calling Kernel.load fixes the issue, however I'm not still able to reproduce the case by creating a custom binstub than just sets it's own signal trap: the signal trap of the executable overrides that of Bundler's.

I suspect that Puma might do some Thread stuff that triggers this edge-case but haven't yet found the end of the rabbit-hole :)

[terlar]

Yes, I wanted to have that simple case example first but also didn't manage to reproduce it with a simple trap. That is why I ended up with puma in my example. I looked into the puma code and it's trap handling but couldn't really wrap my head around it. Thanks for the quick feedback, hope you find the end of the rabbit-hole :)

[asutoshpalai]

I too tried to reproduce it. I went a bit into the puma code and found that it doesn't set a signal trap for catching Interrupt for ruby. What is actually does is, it spans a server tread and waits to join it. To handle INT it catches the exception thrown by join method and "swallows it".

I have abstracted out the model into a small script and the same behavior can be observed.

[agis]

@asutoshpalai Nice explanation, thanks for the digging.

@segiddins I think we should preserve the old behavior by resetting the signal handlers manually (since Kernel.load doesn't). However this is a gotcha that ppl might forget (ie. resetting each new handler that gets added to Bundler's executables). We could also revert the behavior and be consistent. WDYT?

If we want to move forward with the manual reset, I have a fix ready and will wrap up @asutoshpalai's code in a test case and we'll be good to go.

[segiddins]

@agis- I guess we'll just have to restore the old interrupt handler from before bundler installed its when running exec?

[agis]

@segiddins I was thinking something like this: https://github.com/agis-/bundler/commit/dcc05d2585d339adeea3efd0cca6654cb246f212. I still have to figure out a way to test this.

[segiddins]

Oh, if setting it to default works, we should should wipe out every possible signal handler

[agis]

@segiddins AFAICT we only trap INT.

[segiddins]

But that might change ;)

[asutoshpalai]

@agis- happy to help

I attempted at writing a spec for it, it at asutoshpalai 5368aa1. I hope that helps.

It is failing the quality_spec due to use of sleep, but without that the thread persists in background even after rspec finishes.

Edit: Wrong commit link

[asutoshpalai]

Sorry, the spec fails without sleep too ( I mean we can replace sleep with an infinite loop). Maybe it was reminiscent of some other workaround I was trying so that I don't have to modify the helper.

[coilysiren]

@asutoshpalai what's the status on this issue?

[asutoshpalai]

I shall try to make a PR today 🎯