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

Make sure to also rescue non-standard error #269

Merged
merged 3 commits into from
Nov 13, 2019
Merged

Conversation

RusHibaM
Copy link
Contributor

@RusHibaM RusHibaM commented Nov 11, 2019

A potential way to solve #268

I tried to raise the SystemExit again as we have this test case:

raises DeadWorker when using exit so people learn to not kill workers and do not crash main process

If I just rescue all the exceptions, this test will fail since no DeadWorker will be raised when the worker exits.

I'm also thinking about allowing users to specify the non-standard errors that they wish to rescue. By doing this, we may not need to handle all different edge cases and just let the users choose what to rescue.

Parallel.each([1,2,3], extra_exceptions_to_handle: [Exception]) do |index|
end

lib/parallel.rb Outdated Show resolved Hide resolved
@grosser
Copy link
Owner

grosser commented Nov 11, 2019

we'll also need a test that explains the usecase / would not work with the old code

Address the code review comments

Co-Authored-By: Michael Grosser <michael@grosser.it>
@RusHibaM
Copy link
Contributor Author

RusHibaM commented Nov 11, 2019

we'll also need a test that explains the usecase / would not work with the old code

Sure, will add a test case.

BTW, I noticed that Parallel.map can work in isolation was failing in Travis for my changes. I'm a little bit confused as it seems to be passing locally on my machine.

Update:
Ah.. never mind, the Parallel.map can work in isolation is now passing in Travis...

@grosser grosser merged commit 064ae07 into grosser:master Nov 13, 2019
@grosser
Copy link
Owner

grosser commented Nov 13, 2019

1.19.0 ... let's see if this comes back to bite us :D

@RusHibaM
Copy link
Contributor Author

1.19.0 ... let's see if this comes back to bite us :D

Haha, let's see. 😄

@valscion
Copy link

Neat change! I wonder if this list should also have NoMemoryError and Interrupt that should probably not be rescued either. This way the list of "exceptions not to rescue" would match what exceptions RSpec itself does not rescue:

https://github.com/rspec/rspec-support/blob/673133cdd13b17077b3d88ece8d7380821f8d7dc/lib/rspec/support.rb#L132-L140

    module AllExceptionsExceptOnesWeMustNotRescue
      # These exceptions are dangerous to rescue as rescuing them
      # would interfere with things we should not interfere with.
      AVOID_RESCUING = [NoMemoryError, SignalException, Interrupt, SystemExit]

      def self.===(exception)
        AVOID_RESCUING.none? { |ar| ar === exception }
      end
    end

Our usage of parallel won't really care about this, though — I merely spotted this change when updating parallel recently and thought you might want to know about how RSpec does a similar thing ☺️ (PR implementing this in rspec-support originally is here: rspec/rspec-core#2063 in case you're interested)

@grosser
Copy link
Owner

grosser commented Nov 22, 2019

awesome, I copied that in 1.19.1

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.

3 participants