Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

INT handling makes Thin test suite fail #520

Closed
voxik opened this Issue Dec 2, 2011 · 7 comments

Comments

Projects
None yet
4 participants
Contributor

voxik commented Dec 2, 2011

Hello

The following Thin spec runs just fine with RSpec 1.x, however fails with RSpec 2.x

rspec -l 99 spec/daemonizing_spec.rb

The difference is that RSpec 2.x are handling now the INT signal (e521013). Originally, when the forked application received the INT signal, it exited, however, now RSpec handles the signal and they are waiting for second occurrence to quit. Although I have opened this issues at daemons issue tracker [1] (the underlying library used by Thin for daemon support), I am afraid that this might impact other libraries and RSpec should be unobtrusive.

[1] http://rubyforge.org/tracker/index.php?func=detail&aid=29450&group_id=524&atid=2086

Owner

dchelimsky commented Dec 2, 2011

Interesting dilemma. rspec-core captures SIGINT in order to support proper cleanup (i.e. running after hooks, etc) and reporting when you hit CTRL-C. I'd be open to making this configurable so suites like Thin's can turn it off or assign the behavior to a different signal, but I'd want to the default to be as it is.

Contributor

voxik commented Dec 2, 2011

If just single Ctrl+C would be enough, then there would not be any issue IMO. Not sure what is the purpose of it ...

Contributor

justinko commented May 27, 2012

Not sure what is the purpose of it ...

See commit message: e521013

Contributor

voxik commented May 27, 2012

@justinko I saw it, I referenced it in my initial comment.

Contributor

voxik commented Mar 1, 2013

Ping? This is still an issue.

Owner

myronmarston commented Mar 1, 2013

@voxik -- thanks for bringing this to our attention again.

I think @dchelimsky's idea of making the signals rspec captures configurable is the right approach. This also fits in with #741 -- it adds support for a new signal and we want to make that configurable, too.

I'll try to get to this before the next minor release.

Owner

myronmarston commented Mar 23, 2014

Closing in favor of #1439.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment