Popen(preexec_fn=...) issue with polars

[douglas-raillard-arm]

It seems polars 1.15 installs a hook to warn the user about possible deadlocks when using multiprocessing.Pool() with the fork method (default on Unix platforms). However, that hook triggers every time the controls goes back to the Python interpreter after a fork, which unfortunately happens when using the preexec_fn parameter of subprocess.Popen(), which we do.

Do we actually need these preexec_fn ?

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()

devlib/utils/misc.py:156:                                preexec_fn=preexec_function,
devlib/instrument/energy_probe.py:82:                                        preexec_fn=os.setpgrp,
devlib/instrument/arm_energy_probe.py:107:                                       preexec_fn=os.setpgrp,
devlib/host.py:148:        def preexec_fn():
devlib/host.py:157:            preexec_fn=preexec_fn,

The Python documentation states:

Warning

The preexec_fn parameter is NOT SAFE to use in the presence of threads in your application. The child > process could deadlock before exec is called.
https://docs.python.org/3/library/subprocess.html#subprocess.Popen

We very much need to allow having threads in an application using devlib.
Also, the following note provides a possible fix:

Note

If you need to modify the environment for the child use the env parameter rather than doing it in a preexec_fn. The start_new_session and process_group parameters should take the place of code using preexec_fn to call os.setsid() or os.setpgid() in the child.

https://docs.python.org/3/library/subprocess.html#subprocess.Popen
This however was introduced in Python 3.11, so we would either loose compat with older Python versions, or if we want, have a degraded experience on older versions.

In practice from my experimentation, processes are not killed upon Popen garbage collection. However, if the parent process exits due to an exception (e.g. KeyboardInterrupt), then the processes spawned by Popen seem to be terminated, even if the process outlived its Popen handler in the Python world. If the parent process is killed (so exits not via an exception), then the children outlive the parent process.

So to summarize:

1. I don't think the advantages of calling os.setpgrp() as described in preexec_fn comment justifies the risk of undefined behavior outlined in the Python doc, especially when correct resource control on the Python side (e.g. context managers) can allow clean handling in cooperative requests (KeyboardInterrupt).

2. In practice, this conflicts with polars. It can be argued that polars should not install that handler, but really the problem is that Python has a broken default that lets no-one win. This will change in 3.14, but until then we have to deal with it.

So I would suggest we simply remove those preexec_fn without replacement.

The associated polars issue is: pola-rs/polars#20000

[arjpur01]

We faced an issue when using devlib adb background connection in a tool we are developing.
In the tool, we are creating AndroidTarget, and spawning background command -

'

                    cmd = 'devlib-signal-target ' "+ cmd_str
                    r.process = self.target.background(cmd, as_root=True)

'
The command being run is perf stat on the target with arguments corresponding to the different cpu types on the target.
Then, we wait for user to press CTRL + C.

With the exisiting AdbConnection, we are seeing that the CTRL+C pressed is getting intercepted by the adb connection and it gets terminated. We actually have a handling logic that is supposed to terminate the perf using SIGINT, but because of the process intercepting the CTRL+C, that path is not getting taken, and the process has already closed without flushing the data we need.

Adding a preexec function in subprocess.Popen command in android.py of devlib helps to fix the problem.
p: 'Popen' = subprocess.Popen(full_command, stdout=stdout, stderr=stderr, stdin=subprocess.PIPE, shell=True, preexec_fn=os.setpgrp)

Need to see if this is an acceptable generic fix or there needs to be other considerations.

[douglas-raillard-arm]

With the exisiting AdbConnection, we are seeing that the CTRL+C pressed is getting intercepted by the adb connection and it gets terminated.

Something is going on as a (basic) use of Popen and CTRL-C while running shows that this is not the case in a normal scenario:

import time
import subprocess

def main():
    p = subprocess.Popen(['sh', '-c', 'sleep 100h'])
    while True:
        print(p.pid)
        try:
            time.sleep(100000000)
        except BaseException as e:
            print(e)
            pass


main()

[pan-]

I modified the example slightly for clarity:

import time
import subprocess
import os

def print_pstree(msg, p):
    try:
        result = subprocess.run(["pstree", "-p", str(p)], capture_output=True, text=True, check=True)
        print(msg)
        print(result.stdout)
    except subprocess.CalledProcessError as e:
        print("Error calling pstree:", e)

def main():
    wo_session = subprocess.Popen(['sh', '-c', 'sleep 100h'], start_new_session=False)
    with_session = subprocess.Popen(['sh', '-c', 'sleep 100h'], start_new_session=True)
    while True:
        print_pstree("Without session:", wo_session.pid)
        print_pstree("With session:", with_session.pid)
        
        try:
            time.sleep(100000000)
        except BaseException as e:
            print(e)
            pass
main()

The resulting behavior:

Without session:
sh(6817)---sleep(6819)

With session:
sh(6818)---sleep(6821)

^C
Without session:
sh(6817)

With session:
sh(6818)---sleep(6821)

When start_new_session is not set (or False), the sleep command intercepts SIGINT and exits. However, the shell (sh) itself remains alive since it doesn't exit upon receiving SIGINT.

As noted by Arjun, this behavior impacts adb_background_shell because adb intercepts SIGINT but does not propagate it to the remote process (see: source code).

Depending of the use case the code using devlib may want to forward the signal to background processes or block it. To handle properly each use cases, the best option is to let the user do it explicitly which requires to not propagate the signal automatically to the background application (which may just be a proxy...).

Therefore we do need these os.setpgrp or call Popen with start_new_session=True.

To handle this correctly and maintain compatibility, I propose:

• Use start_new_session=True for Python 3.11 and later.
• Continue to use preexec_fn=os.setpgrp for backward compatibility with older Python versions.
• Align the behavior of the AdbConnection background command with the functionality provided by both LocalConnection and SSHConnection.

[douglas-raillard-arm]

Thanks for the analysis Vincent.

1. I just realized start_new_session was actually added in 3.2, only process_group was added in 3.11 so if we only need the former, we are good.
2. For anything like Target.execute(), I don't have strong feelings on whether we should dispatch SIGINT or not to the process as long as an target.execute("sleep 24h") command can be interrupted. I'd assume anything like subprocess.run() would wake itself anyway and kill the process upon receiving SIGINT using the Python signal handlers, so whether the subprocess itself receives it or not is probably irrelevant. We need to check this assumption.
3. For anything like Target.background(), the process should be unaffected and it should be the responsibility of the Python code to decide what to do, as the user may want to implement some sort of loop on the Python side (e.g. wait for Ctrl-C, then do something to get data from target then wait again). We may want to in turn send SIGINT to the command using BackgroundCommand.send_signal() in BackgroundCommand.__exit__ if we were interrupted with KeyboardInterrupt, but maybe not. The current behavior of the context manager's __exit__ would be to close stdin/stdout/stderr and wait for completion.
4. It's not completely clear to me if start_new_session (setsid) is somewhat harmful compared to setpgrp as setsid() does more things. We need to figure out the implication of each solutions.
5. Signal forwarding from shells vary widely and now that you mention it, I vaguely remember coming across that fact about adb. Bash also probably differs from busybox sh and so on ... That problem is what sparked the recently merged work on BackgroundCommand.send_signal(): BackgroundCommand signals don't propagate through some shell setups #645 . We basically can't rely on any (or any absence of) forwarding, even things like sudo and su in the stack can change the behavior.

[douglas-raillard-arm]

Thinking about it, considering shell/sudo/adb behaviors are inconsistent, we may want to simply never forward signals to any subprocess, and then encode the logic in Python so that each connection's execute() method catches KeyboardInterrupt and terminate the subprocess in a way we directly control (unless subprocess.run() does it nicely for us already).

This has been a recurring theme in devlib, where there is a variety of behavior that is one day unified by moving the logic away from connection-specific components and encoding that on the Python side, ideally in Target so it's fully shared (but here we want each connection class to behave well, as their own execute() method is sometimes used directly).

[pan-]

we may want to simply never forward signals to any subprocess, and then encode the logic in Python so that each connection's execute() method catches KeyboardInterrupt and terminate the subprocess in a way we directly control (unless subprocess.run() does it nicely for us already).

In my opinion that is clearly the option to follow as we can consistently "block" signals. Not so much for forwarding them.
Plus there are some applications that will want the signal to be propagated to the child while other application won't want it.

[douglas-raillard-arm]

The main thing to solve with that approach is what do we do in details when we want to actually interrupt the command:

• Target.execute() takes a shell snippet that may contain many commands, plus layers of sh and sudo/su
• BackgroundCommand has a send_signal() method, but due to the same problem as above, only a single process is elected to receive the signal using the shell tool devlib-signal-target. SIGTERM, SIGKILL and SIGQUIT are special-cased though and sent to the process group of one of the "root" shell processes, so it gets delivered to all the processes created by the shell script snippet. We could also decide to deliver a SIGINT to the entire process group in a similar way I think.

[arjpur01]

created a PR - #727 pls review