cap shell dies when Thread.abort_on_exception=true #216

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

@nutrun
Contributor
nutrun commented May 30, 2012

Try this deploy.rb to reproduce: https://gist.github.com/2837590

Patch fixes it by moving the sessions method where processable expects it

Owner

@nutrun thanks for the patch, can you somehow test for this? I don't feel great about accepting thread/exception related patches for a bug I've never heard of, and isn't tested for (because I will probably break this fix unintentionally in the future?)

Thanks

Contributor
nutrun commented May 30, 2012

This test should fail in master: nutrun/capistrano@1bd4467

Contributor
nutrun commented May 30, 2012

To be clear, https://github.com/capistrano/capistrano/blob/master/lib/capistrano/processable.rb#L41 needs a "sessions" instance method, it seems like https://github.com/capistrano/capistrano/blob/master/lib/capistrano/shell.rb#L257 defines it on the Capistrano module instead of Capistrano::Shell

Contributor
nutrun commented Jun 5, 2012

Hi, you reckon the above test is sufficient or do you need more/different tests? The bug is affecting our production use of capistrano and forces us to use our own patched version.

Owner

The test doesn't test the behavior, simply the implementation, I'm not sure that works, I'd like to see a failing test that replicates what actually happens (call a method, raise an exception) in addition to this test. (sorry somehow missed the notification email when you commented on this issue)

Contributor
nutrun commented Jun 15, 2012

I don't mind if you don't merge in my fix, but I reckon the bug should be fixed regardless. I've demonstrated 3 different ways that the bug is there. If this was in a compiled language, the code wouldn't even compile because a method that doesn't exist is being called.

Owner

@nutrun Thanks for your feedback, I'm currently not able to work Capistrnao because of work commitments. Hopefully your project infrastructure is such that you can keep using your own fork until this is merged upstream. I apologize for not getting back to you, I certainly intended to reply to this thread 10 days ago, I really dropped the ball there.

Contributor

@nutrun Looks good to me. Could you take out the unit tests, i.e., go back two commits? This is less of a behavior we want to test and more of a silly mistake.

Contributor
nutrun commented Jul 30, 2012
Contributor

@nutrun I actually meant resetting back to the commit with the fix, thus wiping out the unit tests.

Contributor

@nutrun By the way, thanks for your persistence and patience.

@carsomyr carsomyr closed this Jul 31, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment