Skip to content

[rb] Fix child process terminate method when a process is already terminated #15789

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

Open
wants to merge 25 commits into
base: trunk
Choose a base branch
from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented May 24, 2025

User description

🔗 Related Issues

Original issue #14689

💥 What does this PR do?

This PR adds a rescue for the terminate method on child process so it does not fail when there is nothing to terminate

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Add error handling to terminate for missing processes

  • Introduce unit test for terminate error handling


Changes walkthrough 📝

Relevant files
Bug fix
child_process.rb
Add error handling to terminate method for missing processes

rb/lib/selenium/webdriver/common/child_process.rb

  • Add rescue for Errno::ECHILD and Errno::ESRCH in terminate
  • Prevent errors when terminating non-existent processes
  • +2/-0     
    Tests
    child_process_spec.rb
    Add test for terminate method error handling                         

    rb/spec/unit/selenium/webdriver/common/child_process_spec.rb

  • Add unit test to verify terminate does not raise error on killed
    process
  • Ensure robustness of process termination logic
  • +38/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-rb Ruby Bindings label May 24, 2025
    @qodo-merge-pro qodo-merge-pro bot added Review effort 2/5 and removed C-rb Ruby Bindings labels May 24, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The PR adds error handling for non-existent processes, but should verify if there are other potential errors that could occur during process termination that might need to be handled.

    rescue Errno::ECHILD, Errno::ESRCH
      # Process does not exist, nothing to terminate

    Copy link
    Contributor

    qodo-merge-pro bot commented May 24, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add error handling
    Suggestion Impact:The commit implemented exactly what was suggested - adding error handling for Errno::ECHILD and Errno::ESRCH exceptions in the kill method with the same comment about process not existing

    code diff:

    +      rescue Errno::ECHILD, Errno::ESRCH
    +        # Process does not exist, nothing to kill

    Add error handling to the kill method similar to what was added to terminate.
    This ensures consistent behavior when trying to kill a process that no longer
    exists.

    rb/lib/selenium/webdriver/common/child_process.rb [129-131]

     def kill(pid)
       Process.kill(SIGKILL, pid)
    +rescue Errno::ECHILD, Errno::ESRCH
    +  # Process does not exist, nothing to kill
     end

    [Suggestion processed]

    Suggestion importance[1-10]: 8

    __

    Why: Adding error handling for Errno::ECHILD and Errno::ESRCH in the kill method improves robustness and consistency, preventing unnecessary exceptions when attempting to kill a non-existent process, which is important for reliable process management.

    Medium
    Learned
    best practice
    Add logging for exception handling

    The rescue block has a comment but should include logging to provide better
    visibility when this error condition occurs. This would help with debugging and
    monitoring process termination issues.

    rb/lib/selenium/webdriver/common/child_process.rb [123-127]

     def terminate(pid)
       Process.kill(SIGTERM, pid)
     rescue Errno::ECHILD, Errno::ESRCH
    -  # Process does not exist, nothing to terminate
    +  WebDriver.logger.debug("Process #{pid} does not exist, nothing to terminate", id: :process)
     end
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why:
    Relevant best practice - Add clear error messages and documentation for exception handling

    Low
    • Update

    @aguspe aguspe added the C-rb Ruby Bindings label May 24, 2025
    @aguspe aguspe changed the title Bugfix/rb fix child process error [rb] Fix child process terminate method when a process is already terminated May 24, 2025
    @aguspe aguspe requested a review from p0deje May 26, 2025 18:07
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants