Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround Bundler's friendly errors swallowing error reports #634

Merged
merged 5 commits into from Sep 8, 2020

Conversation

imjoehaines
Copy link
Member

@imjoehaines imjoehaines commented Sep 8, 2020

Goal

When running a script with bundle exec <file> or bundle exec ruby <file>, Bundler will wrap any uncaught exceptions in a SystemExit — see rubygems/rubygems#3368 and friendly_errors.rb

This means that the uncaught exceptions are ignored by default, as we don't report SystemExit exceptions, which can lead to real errors not being reported

This PR attempts to workaround this behaviour by trying to "unwrap" the original exception from the SystemExit. This is made more complicated by it being possible to have two levels of SystemExit exceptions, if an executable script is run directly with bundle exec <file>. In this case we unwrap the "friendly errors" SystemExit and then unwrap the original exception

It's also important that we don't unwrap other SystemExit exceptions, because then we will report any time an exit is used in a rescue block, e.g.

begin
  abc
  xyz
rescue
  exit
end

Design

I'm not a huge fan of inspecting the backtrace for specific files, but they have stayed consistent for years so it should be fairly safe. I wasn't able to find another way to do this that didn't also result in reporting legitimate exits

Changeset

  • added unwrap_bundler_exception which is used in the at_exit hook
  • reworked plain ruby maze runner tests

Testing

The maze runner tests run the same files with the same results using both bundle exec and bundle exec ruby, which proves we can unwrap 1 or 2 levels of exceptions as needed

The new exit_after_exception test ensures we don't report exits outside of Bundler

We avoid reporting SystemExit exceptions as they aren't (usually)
errors. However, Bundler's friendly errors will wrap an exception
in a SystemExit — in this case we need to unwrap to the original
exception
This proves that we don't unwrap exceptions too much when unwrapping
Bundler's friendly errors. In this case, the backtrace will look
something like this:

1. Bundler friendly errors
2. Our exit
3. The exception we raised

We want to drop 1. because it may be hiding a real exception, however
we don't want to drop 2. because it's a deliberate exit

This test proves that we don't go too far and end up with an exception
that we would notify about
@imjoehaines imjoehaines marked this pull request as ready for review September 8, 2020 12:49
@imjoehaines imjoehaines merged commit d9a4afe into next Sep 8, 2020
@imjoehaines imjoehaines deleted the bundler-friendly-errors branch September 8, 2020 16:05
@imjoehaines imjoehaines mentioned this pull request Oct 27, 2020
@deivid-rodriguez
Copy link

deivid-rodriguez commented Nov 16, 2020

Hi! I'm fixing this in rubygems/rubygems#4063.

Just letting you know in case you want to give the fix a try and see if it works for you, and also so that you are aware and can remove the workarounds you had to add.

Regards!

EDIT: I linked to the original issue, not to the PR, updated now :)

@imjoehaines
Copy link
Member Author

Hi! I'm fixing this in rubygems/rubygems#4063.

Just letting you know in case you want to give the fix a try and see if it works for you, and also so that you are aware and can remove the workarounds you had to add.

Regards!

EDIT: I linked to the original issue, not to the PR, updated now :)

Thanks for the heads up! I've done some testing today with Bundler 2.2.4 and your fix does make this workaround unnecessary

We'll keep it around for a while so people have a chance to update Bundler, but thanks for fixing the root issue 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants