[py] Allow `Service` as `command_executor` in `Remote.__init__`

[GideonBear]

User description

🔗 Related Issues

Closes #14464 Ping @cgoldberg

💥 What does this PR do?

Adds Remote(service) as an alternative for Remote(service.service_url). It makes sure the Service doesn't get garbage collected.

Example:

 from selenium import webdriver
 from selenium.webdriver.chrome import service

 def get_driver():
-    global webdriver_service  # This is necessary to avoid dropping the `Service`
-
     webdriver_service = service.Service("operadriver")
     webdriver_service.start()
 
     options = webdriver.ChromeOptions()
     options.add_experimental_option('w3c', True)
 
-    driver = webdriver.Remote(webdriver_service.service_url, options=options)
+    driver = webdriver.Remote(webdriver_service, options=options)
 
     return driver
 
 driver = get_driver()
 driver.get("https://example.com")
 print("Success!")

🔧 Implementation Notes

Added a test that confirms the time at which the Service is dropped, and that any arguments are passed through correctly.

💡 Additional Considerations

Ideally, all code currently using Remote(service.service_url) should change to Remote(service). This could in theory be warned for by Selenium, but that would involve making service_url some kind of str wrapper.

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement

---

Description

• Add Remote.from_service class method for better service lifecycle management

• Prevent service garbage collection by maintaining reference in Remote instance

• Include comprehensive test for service lifecycle and argument passing

---

Diagram Walkthrough

flowchart LR
  Service["Service Instance"] -- "from_service()" --> Remote["Remote WebDriver"]
  Remote -- "maintains reference" --> Service
  Service -- "prevents GC" --> Lifecycle["Service Lifecycle"]

File Walkthrough

	Relevant files
Enhancement	webdriver.py Add from_service class method                                                        py/selenium/webdriver/remote/webdriver.py Add from_service class method to Remote class Import Self type and Service class Store service reference to prevent garbage collection +11/-0
Tests	from_server_tests.py Add comprehensive from_service tests                                          py/test/unit/selenium/webdriver/remote/from_server_tests.py Create test service class with deletion tracking Test Remote subclass for argument validation Verify service lifecycle management and argument passing +48/-0

---

[CLAassistant]

All committers have signed the CLA.

[qodo-code-review[bot]]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 5678 - Partially compliant Compliant requirements: Ensure stable lifecycle/connection handling across multiple driver instantiations. Non-compliant requirements: Diagnose and address repeated "Error: ConnectFailure (Connection refused)" specifics for Ubuntu/ChromeDriver versions mentioned. Provide reproducible behavior where subsequent instances do not fail to connect. Requires further human verification: Validate in real environments (Ubuntu 16.04/Chrome 65/ChromeDriver 2.35) that using Remote.from_service prevents connection failures on subsequent instantiations. Confirm no regressions with existing service lifecycle patterns across browsers. 1234 - Not compliant Non-compliant requirements: Clicking a link with javascript in href should trigger JS in Firefox 42 with Selenium 2.48.x.

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review API Design Storing a private _service reference on Remote without a typed attribute may leak memory if not cleared; confirm lifecycle and document the new API. Consider explicit weakref or cleanup on quit. @classmethod def from_service(cls, service: Service, *args, **kwargs) -> Self: self = cls(service.service_url, *args, **kwargs) # Make sure the service doesn't drop before this drops. # We don't use it, so it shouldn't be defined in types. self._service = service # type: ignore[attr-defined] return self Typing Consistency from_service returns Self but accepts broad *args, **kwargs; ensure constructor signatures across subclasses remain compatible and type checkers won't mislead users. @classmethod def from_service(cls, service: Service, *args, **kwargs) -> Self: self = cls(service.service_url, *args, **kwargs) # Make sure the service doesn't drop before this drops. # We don't use it, so it shouldn't be defined in types. self._service = service # type: ignore[attr-defined] return self

[qodo-code-review[bot]]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Stop service on driver.quit() Remote.from_service retains the Service but WebDriver.quit() doesn’t stop it, which can leave driver processes running until the Remote instance is garbage-collected. Tie the service lifecycle to the driver by calling service.stop() in quit() (when _service is present) and clearing the reference. Add a test asserting the service is stopped immediately after quit() to prevent orphan processes. Examples: py/selenium/webdriver/remote/webdriver.py [283-288] def from_service(cls, service: Service, *args, **kwargs) -> Self: self = cls(service.service_url, *args, **kwargs) # Make sure the service doesn't drop before this drops. # We don't use it, so it shouldn't be defined in types. self._service = service # type: ignore[attr-defined] return self Solution Walkthrough: Before: class WebDriver: @classmethod def from_service(cls, service, *args, **kwargs): self = cls(service.service_url, *args, **kwargs) # The service is retained to prevent garbage collection self._service = service return self def quit(self): # This only sends the QUIT command to the remote end. self.execute(Command.QUIT) # The self._service is not stopped, leaving the process running. After: class WebDriver: @classmethod def from_service(cls, service, *args, **kwargs): self = cls(service.service_url, *args, **kwargs) self._service = service return self def quit(self): try: self.execute(Command.QUIT) finally: # Ensure the service is stopped if it was created via from_service if hasattr(self, "_service"): self._service.stop() Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical resource leak flaw in the PR, where the service process isn't terminated on driver.quit(), and proposes a necessary fix to properly manage the service lifecycle.	High
Learned best practice	Validate service_url before use Validate that service.service_url exists and is a non-empty string before using it, raising a clear error if invalid. py/selenium/webdriver/remote/webdriver.py [282-288] @classmethod def from_service(cls, service: Service, *args, **kwargs) -> Self: - self = cls(service.service_url, *args, **kwargs) + url = getattr(service, "service_url", None) + if not isinstance(url, str) or not url: + raise ValueError("service.service_url must be a non-empty string") + self = cls(url, *args, **kwargs) # Make sure the service doesn't drop before this drops. - # We don't use it, so it shouldn't be defined in types. - self._service = service # type: ignore[attr-defined] + self._service = service return self [ ] Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add defensive checks to validate inputs and required attributes before use.	Low
	✅ Predeclare _service attribute Suggestion Impact: The commit added support for passing a Service to the constructor and dynamically set self._service when a Service is provided. While it did not predeclare self._service in __init__ as suggested, the change clearly addresses handling of the _service attribute, reducing the need for the previous from_service classmethod and its type ignore. code diff: capabilities = options.to_capabilities() _ignore_local_proxy = options._ignore_local_proxy self.command_executor = command_executor + if isinstance(self.command_executor, Service): + # Make sure the service doesn't drop before this drops. + # We don't use it, so it shouldn't be defined in types. + self._service = self.command_executor # type: ignore[attr-defined] + self.command_executor = self.command_executor.service_url if isinstance(self.command_executor, (str, bytes)): self.command_executor = get_remote_connection( capabilities, - command_executor=command_executor, + command_executor=self.command_executor, keep_alive=keep_alive, ignore_local_proxy=_ignore_local_proxy, client_config=client_config, ) Predeclare the _service attribute in init with a default value to avoid dynamically adding it later and remove the need for a type ignore. py/selenium/webdriver/remote/webdriver.py [279-280] self._input = None self._devtools = None +self._service: Optional[Service] = None [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Initialize attributes early to avoid uninitialized or dynamically-added attributes.	Low
[ ] Update

[cgoldberg]

Thanks.. I re-opened the original issue.

hmm... what if we added an additional kwarg to Service, that would control if the service gets stopped when garbage collected?

The default would be True, but you could create a service like:

service.Service("operadriver", auto_stop=False)

Then inside __del__ on the Service class, we don't stop the service when it's created with this flag:

def __del__(self) -> None:
    try:
        if self.auto_stop:
            self.stop()
    except Exception:
        pass

Would that solve the original problem?

[GideonBear]

Would that solve the original problem?

Then the Service would have to be manually stopped, right? That seems like a bad idea. Besides that, my solution solves the issue implicitly, without the user having to be aware of stopping, quitting, etc. .from_service can be used anywhere even when the Service stays in scope. Your idea would not have solved my original problem, which came from a code snippet (copied from webdriver_manager's README to be specific) being copied into a function. Using .from_service in that code snippet (which I will make sure to PR if this is merged :)) would have prevented this; but there would be no reason for the code snippet to use auto_stop=False.

[GideonBear]

That seems like a bad idea.

Small clarification; I don't mean this is a bad idea always, but having to use this to avoid GC is unnecessary IMO. I think my solution results in much cleaner user code, and I would still prefer to see a solution that (as I said) can be applied everywhere; but if that isn't possible that's the next best solution.

[cgoldberg]

Yes, my suggestion would require you to explicitly stop the service.

Your solution adds a classmethod to the base class of all webdrivers that directly instantiates the Service class (an abstract base class). This wouldn't work for any webdriver except Remote.

There might be a nice implicit solution to this, but the current code in your PR isn't it.

[GideonBear]

Your solution adds a classmethod to the base class of all webdrivers that directly instantiates the Service class (an abstract base class). This wouldn't work for any webdriver except Remote.

It does not?? This is selenium.webdriver.remote.webdriver.WebDriver, which is re-exported as selenium.webdriver.Remote.

[GideonBear]

It also does not instantiate a Service. It accepts one as an argument.

[cgoldberg]

It does not??

It does.

selenium.webdriver.remote.webdriver.Webdriver is the base class for all webdrivers. Look at the code for any other webdriver class.

[GideonBear]

You're right, apologies. I expected that to be BaseWebdriver... Sorry, the class hierarchy in Selenium is complexer than I thought.

But that means there is currently no isolated subclass for a Remote, because it is also used a superclass, right? Could a solution be to create a subclass of Remote that is to be used by users, where we replace selenium.webdriver.remote.webdriver.WebDriver with NewRemote as selenium.webdriver.Remote?

[GideonBear]

Then selenium.webdriver.remote.webdriver.WebDriver can also be made an ABC and all users should instead use the NewRemote.

[cgoldberg]

I mean, yea we could reorganize the class hierarchy to make this work. It's just not a trivial change and I don't want to break any existing code relying on the current API. I'm not sure it's worth it just to address a somewhat uncommon use case that we have never really had complaints about before.

[GideonBear]

Yep I get it! I was under the impression that Remote was on the same level as Chrome or Firefox (both in class hierarchy and in uses) but that's not true. It's more of an internal class.

I was on the verge of closing this but one more idea came to me. What if selenium.webdriver.remote.webdriver.WebDriver.__init__'s command_executor argument just takes a Union[str, RemoteConnection, Service]? (previously Union[str, RemoteConnection]) This:

1. Won't break any existing code; there is no overlap between the three types
2. Won't have any impact on subclasses, since they override __init__ and don't get expect a command_executor at all
3. Results in clean user code (Remote(service, options=options); previously Remote(service.service_url, options=options))

[GideonBear]

Implemented, the documentation might need to be improved. I don't know what the standards for types and such are.

[GideonBear]

Rebased. Most previous comments can be ignored, as I pivoted in the implementation.