[py]: More internal refactoring of the base Service class

Author: symonk

py/selenium/webdriver/common/service.py

@@ -16,21 +16,29 @@
 # under the License.
 import contextlib
 import errno
+import logging
 import os
 import subprocess
 import typing
+from abc import ABC
+from abc import abstractmethod
 from platform import system
 from subprocess import DEVNULL
 from subprocess import PIPE
 from time import sleep
+from urllib import request
+from urllib.error import URLError
 
 from selenium.common.exceptions import WebDriverException
 from selenium.webdriver.common import utils
 
+log = logging.getLogger(__name__)
+
+
 _HAS_NATIVE_DEVNULL = True
 
 
-class Service:
+class Service(ABC):
     def __init__(
         self,
         executable: str,
@@ -46,19 +54,20 @@ def __init__(
         # Default value for every python subprocess: subprocess.Popen(..., creationflags=0)
         self.creation_flags = 0
         self.env = env or os.environ
-        self.process = None
+        self.process: typing.Optional[subprocess.Popen] = None
 
     @property
-    def service_url(self):
+    def service_url(self) -> str:
         """
         Gets the url of the Service
         """
         return f"http://{utils.join_host_port('localhost', self.port)}"
 
-    def command_line_args(self):
+    @abstractmethod
+    def command_line_args(self) -> typing.List[str]:
         raise NotImplementedError("This method needs to be implemented in a sub class")
 
-    def start(self):
+    def start(self) -> None:
         """
         Starts the Service.
 
@@ -109,26 +118,30 @@ def start(self):
             count += 1
             sleep(0.5)
             if count == 60:
-                raise WebDriverException("Can not connect to the Service %s" % self.path)
+                raise WebDriverException(f"Can not connect to the Service {self.path}")
 
-    def assert_process_still_running(self):
+    def assert_process_still_running(self) -> None:
+        """Check if the underlying process is still running."""
         return_code = self.process.poll()
         if return_code:
             raise WebDriverException(f"Service {self.path} unexpectedly exited. Status code was: {return_code}")
 
-    def is_connectable(self):
+    def is_connectable(self) -> bool:
+        """Establishes a socket connection to determine if the service running on
+        the port is accessible."""
         return utils.is_connectable(self.port)
 
-    def send_remote_shutdown_command(self):
-        from urllib import request
-        from urllib.error import URLError
-
+    def send_remote_shutdown_command(self) -> None:
+        """
+        Dispatch an HTTP request to the shutdown endpoint for the service in an
+        attempt to stop it.
+        """
         try:
             request.urlopen(f"{self.service_url}/shutdown")
         except URLError:
             return
 
-        for x in range(30):
+        for _ in range(30):
             if not self.is_connectable():
                 break
             sleep(1)
@@ -142,27 +155,27 @@ def stop(self) -> None:
                 # Todo: Be explicit in what we are catching here.
                 self.log_file.close()
 
-        if not self.process:
-            return
-
-        try:
-            self.send_remote_shutdown_command()
-        except TypeError:
-            pass
+        if self.process is not None:
+            with contextlib.suppress(TypeError):
+                self.send_remote_shutdown_command()
+            self._terminate_process()
 
+    def _terminate_process(self) -> None:
+        """Terminate the child process.  On POSIX this attempts a graceful
+        SIGTERM followed by a SIGKILL, on a Windows OS kill is an alias to
+        terminate.  Terminating does not raise itself if something has gone
+        wrong but (currently) silently ignores errors here."""
         try:
-            if self.process:
-                for stream in [self.process.stdin, self.process.stdout, self.process.stderr]:
-                    try:
-                        stream.close()
-                    except AttributeError:
-                        pass
-                self.process.terminate()
-                self.process.wait()
-                self.process.kill()
-                self.process = None
+            stdin, stdout, stderr = self.process.stdin, self.process.stdout, self.process.stdout
+            for stream in stdin, stdout, stderr:
+                with contextlib.suppress(AttributeError):
+                    stream.close()
+            self.process.terminate()
+            self.process.wait(60)
+            # Todo: only SIGKILL if necessary; the process may be cleanly exited by now.
+            self.process.kill()
         except OSError:
-            pass
+            log.error("Error terminating service process.", exc_info=True)
 
     def __del__(self) -> None:
         # `subprocess.Popen` doesn't send signal on `__del__`;