You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
FixesSeleniumHQ#15182
Signed-off-by: Sandro Pischinger <[email protected]>
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:+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.
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VietND96
changed the title
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
Checklist
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 theServiceclass.Changes walkthrough 📝
service.py
Added process termination check in `stop()` methodpy/selenium/webdriver/common/service.py
using
poll().terminated.