diff --git a/devlib/connection.py b/devlib/connection.py index 99055a3c1..e0955b4c8 100644 --- a/devlib/connection.py +++ b/devlib/connection.py @@ -424,6 +424,21 @@ def __enter__(self): self.popen.__enter__() return self + def __exit__(self, *args, **kwargs): + # Send SIGINT to the process group to allow it to clean up if it is running + if self.popen and self.popen.poll() is None: + try: + os.killpg(self.popen.pid, signal.SIGINT) + except Exception: + pass + try: + # allow a graceful termination for 60 seconds + self.popen.wait(timeout=60) + except subprocess.TimeoutExpired: + # If the process did not terminate, send SIGKILL + os.killpg(self.popen.pid, signal.SIGKILL) + self.popen.wait() + class ParamikoBackgroundCommand(BackgroundCommand): """ @@ -638,6 +653,21 @@ def __enter__(self): self.adb_popen.__enter__() return self + def __exit__(self, *args, **kwargs): + # Send SIGINT to the process group to allow it to clean up if it is running + if self.adb_popen.poll() is None: + try: + os.killpg(self.adb_popen.pid, signal.SIGINT) + except Exception: + pass + try: + # allow a graceful termination for 60 seconds + self.adb_popen.wait(timeout=60) + except subprocess.TimeoutExpired: + # If the process did not terminate, send SIGKILL + os.killpg(self.adb_popen.pid, signal.SIGKILL) + self.adb_popen.wait() + class TransferManager: def __init__(self, conn, transfer_poll_period=30, start_transfer_poll_delay=30, total_transfer_timeout=3600): diff --git a/devlib/host.py b/devlib/host.py index a20711cc4..0fe17fdaa 100644 --- a/devlib/host.py +++ b/devlib/host.py @@ -144,8 +144,6 @@ def background(self, command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, as # Make sure to get a new PGID so PopenBackgroundCommand() can kill # all sub processes that could be started without troubles. - def preexec_fn(): - os.setpgrp() def make_init_kwargs(command): popen = subprocess.Popen( @@ -154,7 +152,7 @@ def make_init_kwargs(command): stderr=stderr, stdin=subprocess.PIPE, shell=True, - preexec_fn=preexec_fn, + start_new_session=True, ) return dict( popen=popen, diff --git a/devlib/instrument/arm_energy_probe.py b/devlib/instrument/arm_energy_probe.py index 80ef643da..7aecf90c1 100644 --- a/devlib/instrument/arm_energy_probe.py +++ b/devlib/instrument/arm_energy_probe.py @@ -102,6 +102,9 @@ def reset(self, sites=None, kinds=None, channels=None): def start(self): self.logger.debug(self.command) + # FIXME - replace this preexec_fn arg with start_new_session argument. + # to address https://github.com/ARM-software/devlib/issues/708. + # didnt do it with initial change for the fix due to lack of test hardware self.armprobe = subprocess.Popen(self.command, stderr=self.output_fd_error, preexec_fn=os.setpgrp, diff --git a/devlib/instrument/energy_probe.py b/devlib/instrument/energy_probe.py index 6a97c5bf2..bb56ed62a 100644 --- a/devlib/instrument/energy_probe.py +++ b/devlib/instrument/energy_probe.py @@ -75,6 +75,9 @@ def reset(self, sites=None, kinds=None, channels=None): def start(self): self.logger.debug(self.command) + # FIXME - replace this preexec_fn arg with start_new_session argument. + # to address https://github.com/ARM-software/devlib/issues/708. + # didnt do it with initial change for the fix due to lack of test hardware self.process = subprocess.Popen(self.command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, diff --git a/devlib/utils/android.py b/devlib/utils/android.py index 2be37cccd..ce00d1e7e 100755 --- a/devlib/utils/android.py +++ b/devlib/utils/android.py @@ -687,7 +687,8 @@ def with_uuid(cmd): adb_cmd = get_adb_command(device, 'shell', adb_server, adb_port) full_command = f'{adb_cmd} {quote(command)}' logger.debug(full_command) - p = subprocess.Popen(full_command, stdout=stdout, stderr=stderr, stdin=subprocess.PIPE, shell=True) + p = subprocess.Popen(full_command, stdout=stdout, stderr=stderr, stdin=subprocess.PIPE, shell=True, + start_new_session=True) # Out of band PID lookup, to avoid conflicting needs with stdout redirection grep_cmd = f'{busybox} grep {quote(command_uuid)}' diff --git a/devlib/utils/misc.py b/devlib/utils/misc.py index 1c49d0d0b..f523f410a 100644 --- a/devlib/utils/misc.py +++ b/devlib/utils/misc.py @@ -136,14 +136,6 @@ def get_cpu_name(implementer, part, variant): return name -def preexec_function(): - # Change process group in case we have to kill the subprocess and all of - # its children later. - # TODO: this is Unix-specific; would be good to find an OS-agnostic way - # to do this in case we wanna port WA to Windows. - os.setpgrp() - - check_output_logger = logging.getLogger('check_output') def get_subprocess(command, **kwargs): @@ -153,7 +145,7 @@ def get_subprocess(command, **kwargs): stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, - preexec_fn=preexec_function, + start_new_session=True, **kwargs)