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 no error is raised when a process is already terminated #939

Merged

Conversation

louim
Copy link
Contributor

@louim louim commented Mar 1, 2024

Hey! Sometime when using parallel_rspec --fail-fast --test-options '--fail-fast', it can happen that the process is already gone before stop_all_processes has time to kill it. In this case Errno::ESRCH was raised. Rescuing the error ensures that the exits happen as expected for the parent process itself.

Fix errors like that:

bundler: failed to load command: parallel_rspec (/Users/louim/.asdf/installs/ruby/3.0.6/bin/parallel_rspec)
/Users/louim/.gem/ruby/3.0.0/gems/parallel_tests-4.2.1/lib/parallel_tests.rb:46:in `kill': No such process (Errno::ESRCH)
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel_tests-4.2.1/lib/parallel_tests.rb:46:in `block in stop_all_processes'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel_tests-4.2.1/lib/parallel_tests.rb:46:in `each'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel_tests-4.2.1/lib/parallel_tests.rb:46:in `stop_all_processes'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel_tests-4.2.1/lib/parallel_tests/cli.rb:63:in `block (4 levels) in execute_in_parallel'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel-1.23.0/lib/parallel.rb:627:in `call_with_index'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel-1.23.0/lib/parallel.rb:418:in `block (2 levels) in work_in_threads'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel-1.23.0/lib/parallel.rb:637:in `with_instrumentation'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel-1.23.0/lib/parallel.rb:417:in `block in work_in_threads'
	from /Users/louim/.gem/ruby/3.0.0/gems/parallel-1.23.0/lib/parallel.rb:219:in `block (4 levels) in in_threads'

Wasn't sure a changelog entry was required. Let me know if you'd like one.

Checklist

  • Feature branch is up-to-date with master (if not - rebase it).
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • Update Readme.md when cli options are changed

Copy link
Owner

@grosser grosser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thx!

@grosser
Copy link
Owner

grosser commented Mar 2, 2024

expected no Exception, got #<Errno::EINVAL: Invalid argument> with backtrace:
CI is not happy, can you fix that ?

@louim
Copy link
Contributor Author

louim commented Mar 2, 2024

Seems to be Windows related, I’ll have a look.

@louim louim force-pushed the improvement/no-raise-on-missing-process branch from 4244c00 to f09259c Compare March 4, 2024 23:03
@louim
Copy link
Contributor Author

louim commented Mar 4, 2024

@grosser I ended up skipping the test on Windows, like the one right above the one I added: 92374ee

I hope that's ok.

Copy link
Owner

@grosser grosser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice ... did not see that :D

@grosser
Copy link
Owner

grosser commented Mar 5, 2024

still a failure ... do we need to include Errno::EPERM ?

@louim louim force-pushed the improvement/no-raise-on-missing-process branch from f09259c to c5eb414 Compare March 5, 2024 03:16
@louim
Copy link
Contributor Author

louim commented Mar 5, 2024

@grosser I don't think it should be required. My guess is that the random number I choose for the PID is conflicting with something actually running for that combination of ruby/os on the CI. I wasn't able to reproduce locally. Let's see how it does with a different number.

@grosser grosser merged commit a0373af into grosser:master Mar 5, 2024
13 checks passed
@grosser
Copy link
Owner

grosser commented Mar 5, 2024

4.5.2

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

2 participants