Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popen(preexec_fn=...) issue with polars #708

Open
douglas-raillard-arm opened this issue Nov 26, 2024 · 0 comments
Open

Popen(preexec_fn=...) issue with polars #708

douglas-raillard-arm opened this issue Nov 26, 2024 · 0 comments

Comments

@douglas-raillard-arm
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant