[py] service: only shutdown if process not terminated

[PSandro]

User description

There are scenarios where a stop() is called on a Service object multiple times. This also happens when explicitly quitting a Webdriver using driver.quit() and afterwards having the garbage collector destroy the service object, calling stop() another time even though the service process has already terminated.

The check inside the stop() call only ensured that the process variable is not None, but ignored the fact that the process might already have terminated. Therefor an additional check is introduced to only send the remote shutdown command if the service process has not ended.

Motivation and Context

Fixes #15182

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix

---

Description

• Added a check to ensure the service process has not terminated before sending a shutdown command.

• Prevented redundant shutdown attempts when stop() is called multiple times.

• Improved robustness of the stop() method in the Service class.

---

Changes walkthrough 📝

	Relevant files
Bug fix	service.py Added process termination check in `stop()` method              py/selenium/webdriver/common/service.py Added a condition to check if the service process is still active using poll(). Prevented shutdown command execution if the process has already terminated. +1/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The new condition checks if process is not None and not terminated, but there could still be race conditions where process terminates between the check and send_remote_shutdown_command call if self.process is not None and self.process.poll() is None:

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add error handling for process check Add error handling for the case when process.poll() raises an exception, which can happen if the process object is in an invalid state. py/selenium/webdriver/common/service.py [155] -if self.process is not None and self.process.poll() is None: +if self.process is not None: + try: + if self.process.poll() is None: + try: + self.send_remote_shutdown_command() + except TypeError: + pass + except Exception: + pass Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion addresses a potential robustness issue by adding error handling for process.poll(), which could fail if the process object is in an invalid state. This is a meaningful defensive programming improvement for process management.	7
Learned best practice	Move null checks to the start of methods for early validation and clearer control flow Move the self.process null check to the start of the method to fail fast and handle the null case explicitly, rather than nesting it within the log output handling logic. This makes the code clearer and prevents unnecessary log handling for null processes. py/selenium/webdriver/common/service.py [152-158] def stop(self) -> None: + if self.process is None: + return + if self.log_output not in {PIPE, subprocess.DEVNULL}: if isinstance(self.log_output, IOBase): self.log_output.close() elif isinstance(self.log_output, int): os.close(self.log_output) - if self.process is not None and self.process.poll() is None: + if self.process.poll() is None: try: self.send_remote_shutdown_command() except TypeError: pass Apply this suggestion	6

[VietND96]

@shbenzer can you take a look once?

[shbenzer]

@navin772 was unable to reproduce the error in #15182

[PSandro]

@navin772 was unable to reproduce the error in #15182

Will add more details and information in the issue thread. Even if the delayed termination cannot be reproduced (yet), this PR still includes an improvement. When trying to reproduce the described behavior you will be able to observe that the "shutdown" request is sent twice: once when calling quit() on the driver object and once when the driver object gets cleaned up. Even if the (2min) blocking of the second "shutdown" request cannot be reproduced, the second "shutdown" request is unwanted nevertheless - which is resolved by this PR.