From 6ace72e4e7f8c626be609b8d947d81cfe8f0b83f Mon Sep 17 00:00:00 2001 From: Kazuaki Matsuo Date: Sat, 31 Oct 2026 09:08:56 -0700 Subject: [PATCH 1/2] refactor: consolidate the process end handling --- appium/webdriver/appium_service.py | 49 ++++++++++++---------- test/unit/webdriver/appium_service_test.py | 31 ++++++++++++++ 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/appium/webdriver/appium_service.py b/appium/webdriver/appium_service.py index ab37fd84..74733eb1 100644 --- a/appium/webdriver/appium_service.py +++ b/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,36 @@ 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.""" + assert self._process + process = self._process + 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: diff --git a/test/unit/webdriver/appium_service_test.py b/test/unit/webdriver/appium_service_test.py index 360f0ed6..596ae51e 100644 --- a/test/unit/webdriver/appium_service_test.py +++ b/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() From 8b7ca46d76e89fb8742d97cca485c903cebea355 Mon Sep 17 00:00:00 2001 From: Kazuaki Matsuo Date: Sat, 31 Oct 2026 09:10:20 -0700 Subject: [PATCH 2/2] return none --- appium/webdriver/appium_service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/appium/webdriver/appium_service.py b/appium/webdriver/appium_service.py index 74733eb1..0c0031cd 100644 --- a/appium/webdriver/appium_service.py +++ b/appium/webdriver/appium_service.py @@ -156,8 +156,9 @@ def stop(self, timeout: float = STOP_TIMEOUT_SEC) -> bool: def _terminate_process(self, timeout: float) -> bytes | str | None: """Terminate the process and collect stderr within bounded waits.""" - assert self._process process = self._process + if process is None: + return None try: process.terminate() try: