Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Avoid bundle exec rescue of SignalException #6092

Merged
merged 2 commits into from
Oct 18, 2017

Conversation

dekellum
Copy link
Contributor

Avoid rescue of SignalException in kernel_load and with_friendly_errors This allows SignalException to be handled gracefully by the ruby interpreter.

This supersedes #6091.

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

Fixes #6090

What was your diagnosis of the problem?

CLI::Exec#kernel_load should not rescue the SignalException. Neither should with_friendly_errors, so it unwinds all the way to the ruby interpreter.

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

Two rescue and re-raises specific to SignalException.

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

My first naive attempt was #6091, where I had missed the outer rescue in with_friendly_errors, all of the other special friendly error behavior, and the spec dependencies. While I still suspect that kernel_load and friendly_errors might be rescuing/handling more than they should (for comparison: binstub doesn't do this) this is a much more minimal, safer, and easier fix to #6090.

@dekellum
Copy link
Contributor Author

I think the Travis CI failure for one variant is spurious ("RuntimeError: Digest::Base cannot be directly inherited in Ruby") but I don't seem to have a rebuild option, and I'm going to try to resist the temptation of making a cosmetic commit to achieve the same.

@dekellum
Copy link
Contributor Author

Looks like #6084 is in progress to resolve this same CI failure.

@olleolleolle
Copy link
Member

@dekellum That other Digest-related change has been merged - now master builds green again.

This allows SignalException to be handled gracefully by the ruby
interpreter.

(rebased on to master with CI Fix, 6084)

github: fixes rubygems#6090
@dekellum dekellum force-pushed the do-not-rescue-signal-exception branch from 7a79f52 to a0c1d73 Compare October 14, 2017 13:40
@dekellum
Copy link
Contributor Author

Yes, thanks all, CI build now passing with rebase.

@segiddins
Copy link
Member

Can you please add a test in spec/commands/exec_spec.rb that will ensure we treat executables that raise SignalExceptions properly no matter which form of execution is used? Thanks!

@dekellum
Copy link
Contributor Author

I read that spec file, and adding a spec for SignalException sounds reasonable and should be straightforward. Unfortunately I am unable to get the specs working at all in my local environment, see #6105. To expedite this bug fix, perhaps someone who can run the specs locally could attempt this?

@segiddins
Copy link
Member

In the meantime, you can run the individual file with bin/rspec spec/commands/exec_spec.rb

@dekellum
Copy link
Contributor Author

dekellum commented Oct 16, 2017

I get a different failure (Gem::Indexer requires that the XML Builder library be installed) if I try that. See addition in #6105. Also note, I tried installing both builder 2.1.2 and 3.2.3 to workaround this. No luck.

dekellum added a commit to dekellum/bundler that referenced this pull request Oct 17, 2017
@dekellum
Copy link
Contributor Author

dekellum commented Oct 18, 2017

Lacking a workaround for #6105, this was bit of a painful process using Travis as my dev. environment, setup against my own repo. But the test in b10910a passes Bundler's CI. Could you review and let me know if this is sufficient? Note that only the Kernel::load path is relevant to #6090, and that is the path that is tested here.

@segiddins
Copy link
Member

Thanks so much for sticking with it <3

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit b10910a has been approved by segiddins

@bundlerbot
Copy link
Collaborator

⌛ Testing commit b10910a with merge 6d0b74c...

bundlerbot added a commit that referenced this pull request Oct 18, 2017
…ddins

Avoid bundle exec rescue of SignalException

Avoid rescue of SignalException in kernel_load and with_friendly_errors This allows SignalException to be handled gracefully by the ruby interpreter.

This supersedes #6091.

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

Fixes #6090

### What was your diagnosis of the problem?

CLI::Exec#kernel_load should not rescue the SignalException. Neither should with_friendly_errors, so it unwinds all the way to the ruby interpreter.

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

Two rescue and re-raises specific to SignalException.

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

My first naive attempt was #6091, where I had missed the outer rescue in with_friendly_errors, all of the other special friendly error behavior, and the spec dependencies. While I still suspect that kernel_load and friendly_errors might be rescuing/handling more than they should (for comparison: binstub doesn't do this) this is a much more minimal, safer, and easier fix to #6090.
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: segiddins
Pushing 6d0b74c to master...

@bundlerbot bundlerbot merged commit b10910a into rubygems:master Oct 18, 2017
@segiddins segiddins added this to the 1.16.0 milestone Oct 20, 2017
segiddins pushed a commit that referenced this pull request Oct 30, 2017
…ddins

Avoid bundle exec rescue of SignalException

Avoid rescue of SignalException in kernel_load and with_friendly_errors This allows SignalException to be handled gracefully by the ruby interpreter.

This supersedes #6091.

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

Fixes #6090

### What was your diagnosis of the problem?

CLI::Exec#kernel_load should not rescue the SignalException. Neither should with_friendly_errors, so it unwinds all the way to the ruby interpreter.

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

Two rescue and re-raises specific to SignalException.

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

My first naive attempt was #6091, where I had missed the outer rescue in with_friendly_errors, all of the other special friendly error behavior, and the spec dependencies. While I still suspect that kernel_load and friendly_errors might be rescuing/handling more than they should (for comparison: binstub doesn't do this) this is a much more minimal, safer, and easier fix to #6090.

(cherry picked from commit 6d0b74c)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bundle exec rescues 'SignalException' and aborts
4 participants