[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.

[cgoldberg]

@PSandro
I like this change. This isn't a common scenario, but poll() will return an exit code if the process was already terminated by a signal (in which case, we also don't want to send the shutdown command).

How about this to handle that case too:

retcode = self.process.poll()
if self.process is not None and (retcode is None or isinstance(retcode, int)):
    try:
        self.send_remote_shutdown_command()
    except TypeError:
        pass
    finally:
        self._terminate_process()

I also moved _terminate_process() into a finally block so it gets called even when the shutdown raises a TypeError (although I have no idea what would cause that)

[shbenzer]

@PSandro I like this change. This isn't a common scenario, but poll() will return an exit code if the process was already terminated by a signal (in which case, we also don't want to send the shutdown command).

How about this to handle that case too:

retcode = self.process.poll()
if self.process is not None and (retcode is None or isinstance(retcode, int)):
    try:
        self.send_remote_shutdown_command()
    except TypeError:
        pass
    finally:
        self._terminate_process()

I also moved _terminate_process() into a finally block so it gets called even when the shutdown raises a TypeError (although I have no idea what would cause that)

The finally is definitely cleaner. does poll return other codes?

edit: poll returns none unless termination or an error (causing termination) occurs, in which cases int. Shutdown should be just on None return. Current code LGTM

[cgoldberg]

It's actually used twice... that's the only reason I saved it in a variable :)

[cgoldberg]

See my comment regarding return code.

[shbenzer]

It's actually used twice... that's the only reason I saved it in a variable :)

I caught it immediately after sending the message (had to scroll on my phone) - you responded before I could edit lol

[cgoldberg]

@shbenzer actually, my suggestion is incorrect. poll returns None if the process is still running (otherwise a return code). So the original code is correct, not mine.

@PSandro I think the only change needed is to move that code into the finally block like my example... otherwise this looks good

[shbenzer]

@shbenzer actually, my suggestion is incorrect. poll returns None if the process is still running (otherwise a return code). So the original code is correct, not mine.

Yep, I mentioned that in my comment edit. Your finally addition is cleaner, so I’d like to have that added though.

[PSandro]

Thank you for the review. How does it look to you now?

[cgoldberg]

@PSandro sorry for the confusion... Could you revert the suggestion I made and keep it the way you had it in your original PR? (However, leave the finally block in place).

[PSandro]

@PSandro sorry for the confusion... Could you revert the suggestion I made and keep it the way you had it in your original PR? (However, leave the finally block in place).

@cgoldberg sure. If I'm not mistaken, then this is actually the case. Then only changes in this branch are original addition self.process.poll() is None: and the added finally block around self._terminate_process() following your suggestion. I cannot see any other suggestion/commit applied to this branch.

$ git log --oneline  py/selenium/webdriver/common/service.py                                                                                                                        0
eb19349553 service: move process termination to finally block
1eb1fbdfc9 service: only shutdown if process not terminated

Feel free to request additional changes if wanted 👍

[cgoldberg]

Looks good. Thanks!

[titusfortner]

Thanks for the PR!