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

:process_incoming_jobs doesn't rescue non-standard error, which could lead to DeadWorker #268

Open
RusHibaM opened this issue Nov 7, 2019 · 2 comments

Comments

@RusHibaM
Copy link
Contributor

RusHibaM commented Nov 7, 2019

Thanks in advance for making this Gem 😄. Our team has been enjoying it for a long time.~

However, as we also use Rspec in our automation systems, I found that sometimes our test would fail with a DeadWorker. This DeadWorker makes our investigation a little bit difficult. By digging into your library code more, I found that the fact that our tests would fail with DeadWorker is a consequence of:

  1. When something like expect{a}.to be b fail, Rspec will raise a RSpec::Expectations::ExpectationNotMetError, which isn't a StandardError
  2. We are using Parallel with in_processes so many of our code will be executed on a forked process.
  3. The rescue in https://github.com/grosser/parallel/blob/master/lib/parallel.rb#L472 doesn't seem to catch the RSpec::Expectations::ExpectationNotMetError as it isn't a StandardError.
  4. The process died out, and the worker failed to read the file. When this happened, DeadWorker from (https://github.com/grosser/parallel/blob/master/lib/parallel.rb#L74) will be the final thing recorded.

I have a small example so you could also have a try:

require 'rspec/expectations'
require 'parallel'

# This line will fail and the last thing in the rspec report is a Parallel::DeadWorker
Parallel.each(1..1, in_processes:1) {|x| raise RSpec::Expectations::ExpectationNotMetError}

# This line will leave the correct exception as the exception happens in the main process and 'kills' the process
Parallel.each(1..1, in_threads:1) {|x| raise RSpec::Expectations::ExpectationNotMetError}

I tried to make a small change that lets the rescue (https://github.com/grosser/parallel/blob/master/lib/parallel.rb#L472) to catch Exception and after the changes, our annoying DeadWorker was resolved.

As I don't really know the designing details of process_incoming_jobs, this Issue is mainly for asking:

  1. Are there any concerns so that you guys didn't choose to rescue all the exceptions in process_incoming_jobs?
  2. We could patch what we need in our own system. But do you think it is worthy to modify the process_incoming_jobs so it could catch Exception? Or is there any other thing that might be better? We benefit from the open source community a lot so I would like to submit a PR for this. Feels like this could be something that everyone could run into, especially when using libraries such as Rspec that raises non-StandardError.
@grosser
Copy link
Owner

grosser commented Nov 9, 2019

Yeah it might be a good idea to rescue all ... since it's done in a fork and when we don't things get weird ...
The initial implementation was done because it is a common best practice to only rescue StandardError and I did not want to have to think through all the weird edge-cases.

Please try a PR that rescues Exception instead and see if the tests are happy with that.

@RusHibaM
Copy link
Contributor Author

Thanks for reply~ I see. Right, it is a common best practice to only rescue StandardError. All tests passed on my local machine so I have created a PR #269.

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

No branches or pull requests

2 participants