refactor: consolidate the process end handling

appium/webdriver/appium_service.py

@@ -127,17 +127,7 @@ def start(self, **kwargs: Any) -> sp.Popen:
             if stderr == sp.PIPE and self._process.stderr is not None:
                 # The server might still be running, for example if it does not listen on the expected URL.
                 # Reading its stderr before the process exits would block forever, so terminate it first.
-                self._process.terminate()
-                try:
-                    _, err_output = self._process.communicate(timeout=STOP_TIMEOUT_SEC)
-                except sp.TimeoutExpired:
-                    # The server ignored the termination request. Kill it, so its error output is not lost.
-                    self._process.kill()
-                    try:
-                        _, err_output = self._process.communicate(timeout=STOP_TIMEOUT_SEC)
-                    except sp.TimeoutExpired as e:
-                        # Another process might still hold stderr open. Keep the output read so far.
-                        err_output = e.stderr or b''
+                err_output = self._terminate_process(STOP_TIMEOUT_SEC)
                 if err_output:
                     error_msg += f'\nOriginal error: {str(err_output)}'
             self.stop()
@@ -156,21 +146,37 @@ def stop(self, timeout: float = STOP_TIMEOUT_SEC) -> bool:
         Returns:
             `True` if the service was running before being stopped
         """
-        was_running = False
-        if self.is_running:
-            assert self._process
-            was_running = True
-            self._process.terminate()
+        was_running = self.is_running
+        if was_running:
+            self._terminate_process(timeout)
+        else:
+            self._process = None
+            self._cmd = None
+        return was_running
+
+    def _terminate_process(self, timeout: float) -> bytes | str | None:
+        """Terminate the process and collect stderr within bounded waits."""
+        process = self._process
+        if process is None:
+            return None
+        try:
+            process.terminate()
             try:
-                self._process.communicate(timeout=timeout)
+                _, stderr = process.communicate(timeout=timeout)
             except sp.SubprocessError:
                 if sys.platform == 'win32':
-                    sp.call(['taskkill', '/f', '/pid', str(self._process.pid)])
+                    sp.call(['taskkill', '/f', '/pid', str(process.pid)])
                 else:
-                    self._process.kill()
-        self._process = None
-        self._cmd = None
-        return was_running
+                    process.kill()
+                try:
+                    _, stderr = process.communicate(timeout=timeout)
+                except sp.TimeoutExpired as e:
+                    # Another process might still hold stderr open. Keep the output read so far.
+                    stderr = e.stderr
+            return stderr
+        finally:
+            self._process = None
+            self._cmd = None
 
     @property
     def is_running(self) -> bool:

test/unit/webdriver/appium_service_test.py

@@ -134,3 +134,34 @@ def kill():
 
         assert f'Original error: {expected_error}' in str(excinfo.value)
         process.kill.assert_called_once()
+
+    @pytest.mark.parametrize('platform', ['win32', 'linux'])
+    @pytest.mark.parametrize('timeouts', [0, 1, 2])
+    def test_stop_cleans_up_after_termination(self, monkeypatch, platform, timeouts):
+        process = Mock()
+        process.poll.return_value = None
+        process.communicate.side_effect = [appium_service.sp.TimeoutExpired('appium', 1)] * timeouts + [(b'', b'')]
+        taskkill = Mock()
+        monkeypatch.setattr(appium_service.sys, 'platform', platform)
+        monkeypatch.setattr(appium_service.sp, 'call', taskkill)
+        monkeypatch.setattr(appium_service.sp, 'Popen', Mock(return_value=process))
+        service = AppiumService()
+        service.start(node='node', npm='npm', main_script='appium.js', timeout_ms=0)
+
+        assert service.stop(timeout=1) is True
+
+        process.terminate.assert_called_once()
+        assert process.communicate.call_args_list == [call(timeout=1)] * (1 if timeouts == 0 else 2)
+        if timeouts and platform == 'win32':
+            taskkill.assert_called_once_with(['taskkill', '/f', '/pid', str(process.pid)])
+            process.kill.assert_not_called()
+        elif timeouts:
+            process.kill.assert_called_once()
+            taskkill.assert_not_called()
+        else:
+            process.kill.assert_not_called()
+            taskkill.assert_not_called()
+        assert not service.is_running
+        assert service._cmd is None
+        assert service.stop() is False
+        process.terminate.assert_called_once()