pipx run isn't redirecting signals to child app (?)

[gmagno]

Hello,

What a wonderful tool, no more manual management of python apps in my system :)

About the issue, I wonder if signals are being redirected to the child app when calling pipx run <app>. For instance:

pipx run --spec tmonpy tmon

tmon in the absence of arguments, waits for SIGINT to return and print a temperature report. But it looks like pipx run when SIGINT-ed kills the app and doesn't give it a chance to return gracefully (I could be wrong though, I haven't looked at the source).

So this is what I would expect to happen:

pipx run --spec tmonpy tmon

wait a few seconds, press Ctrl+C and a graph should be printed in the terminal:

===================
Temp Monitor Report

    temp (°C) for a period of 0:00:04
       41.00  ┤
       40.86  ┤
       40.71  ┤
       40.57  ┤
       40.43  ┤
       40.29  ┤
       40.14  ┤
       40.00  ┼╮
       39.86  ┤│
       39.71  ┤│
       39.57  ┤│
       39.43  ┤│
       39.29  ┤│
       39.14  ┤│
       39.00  ┤╰───

    min: 39.1 °C
    avg: 39.2 °C
    max: 40.0 °C

    raw: /tmp/[email protected]
===================

Note: tmon is expected to work on Linux only! Don't try to run the test case on mac or win.

[itsayellow]

I'm not sure I'm the expert but I can give some help on a few things.

We use subprocess.run() to run your pipx run script, and we do not supply a stdin argument, which should mean that stdin is inherited from the parent process. If I understand the subprocess mechanics correctly, this should mean it also inherits all of the signals like SIGINT from the parent process. Although I'm a little fuzzy on how subprocess handles signals.

From my reading it appears we don't specifically try and prevent signals from reaching the subprocess, but it is possible we need to do something else.

BTW I did find this which may be relevant:
https://bugs.python.org/issue25942
Which version of python are you using? The above bug seems to be python < 3.7 and the problem seems to be that it immediately sends a SIGKILL not giving the python app time to handle a SIGINT.

[gmagno]

We use subprocess.run() to run your pipx run script, and we do not supply a stdin argument, which should mean that stdin is inherited from the parent process. If I understand the subprocess mechanics correctly, this should mean it also inherits all of the signals like SIGINT from the parent process.

That sounds sensible to me.

I can't rule out a bug on tmon. To be sure a simpler test case should probably be used. OTOH tmon works fine when run from the terminal, which leads to the second part of your comment and the bug report you found, but I'm using python3.8, so that shouldn't be an issue :thinking:

[uranusjr]

I just tested this on 3.8. It is true that the subprocess is not killed immediately and has a change to handle SIGINT, but the child is still killed after it handles SIGINT by the KeyboardInterrupt exception. My guess is that tmon probably got the signal, but wasn’t able to print stuff in time before KeyboardInterrupt terminated it (although I didn’t manage to get the package running in WSL to actually test it out).

This seems to do the job:

import signal
import subprocess
import sys

# Without this line, CTRL-C would kill the subprocess launched in the next block.
sigint_handler = signal.signal(signal.SIGINT, signal.SIG_IGN)

try:
    # Now CTRL-C does not kill the subprocess. CTRL-D or exit() ends the interactive shell.
    subprocess.run([sys.executable])
finally:
    # Restore signal handler responsibly.
    signal.signal(signal.SIGINT, sigint_handler)

[itsayellow]

@uranusjr , so you're saying use the code you provided to wrap pipx's call of subprocess.run()?

Am I correct in reading this code as "disable SIGINT from affecting the parent pipx process while running the subprocess, then re-enable SIGINT after the subprocess has exited"?

If that is what's happening, don't we have to worry about providing a timeout, etc., for all the same reasons that were debated in the python bug?

[gmagno]

My guess is that tmon probably got the signal, but wasn’t able to print stuff in time

If I change tmon main loop sleep time from 1 to 0.1, here, tmon is able to return gracefully, which reiterates your guess.
~Now, pipx run shouldn't have to endlessly wait for the child process, should there be a mechanism of specifying a timeout?~

Edit: Actually pipx run should in fact endlessly wait for the child process. If, for instance the child process ignores SIGINT, why should pipx run override that behaviour and kill it? I believe pipx run should wait for signal.SIGCHLD, and only then return.

[itsayellow]

The impression I get from that python bug report is that they wait 1 second before issuing a SIGKILL. Which would be consistent.

[gmagno]

The impression I get from that python bug report is that they wait 1 second before issuing a SIGKILL. Which would be consistent.

I think that's sensible if we're talking about how subprocess should handle it. And potentially expose a timeout parameter (which already does) so the user can configure that behaviour. But my guess would be that pipx run shouldn't do that, for the reasons I mention in my previous comment "Edit".

[cs01]

We should be using something like execvp instead of subprocess calls for pipx run. Then it should be indistinguishable from running it without pipx. If someone can test it out that would be helpful. I have been really busy so I don’t think I’ll have any time to. https://docs.python.org/3/library/os.html#os.execl

[uranusjr]

I have a block of code implementing pipenv run with exec* on POSIX and subprocess on Windows that should be useful.

[uranusjr]

Am I correct in reading this code as "disable SIGINT from affecting the parent pipx process while running the subprocess, then re-enable SIGINT after the subprocess has exited"?

SIGINT is still sent to the child when you disable it in the parent, so in general it should be able to make the subprocess shutdown when CTCL-C pressed. If the child traps SIGINT and does not shutdown on that at all, the app would be problematic anyway and that’s arguably not pipx’s job to protect against it.

[itsayellow]

@uranusjr do you have a link to the pipenv exec* code?

[uranusjr]

This file encapsulates cross-platform argument parsing: https://github.com/pypa/pipenv/blob/cda15b3b30e04e038ee286bced6c47a311f1e0ec/pipenv/cmdparse.py

And then the object is used to launch the process. On POSIX it’s basically

os.execl(shutil.which(script.command), script.command, *script.args)

The Windows implementation is a bit more involved: https://github.com/pypa/pipenv/blob/cda15b3b30e04e038ee286bced6c47a311f1e0ec/pipenv/core.py#L2444-L2470

(system_which can be replaced by shutil.which() for pipx)

[itsayellow]

I've been playing around with a prototype using os.exec*, and it functionally works great.

I am having trouble with testing, however. pytest doesn't seem register any tests finishing if they exec* another process. Any hints how to make os.exec* compatible with pytest and our testing?

[cs01]

I would suggest mocking it out as a fixture, and calling subprocess.run instead as we have been. Then there can be a single test that verifies os.exec is called, again while it is mocked out.

[itsayellow]

I can definitely mock the called function that uses os.exec to use subprocess.run instead.

Just not exactly sure how to do the part that verifies os.exec, pytest really seems to lose track of it once it doesn't finish as part of the original python process...

[uranusjr]

os.exec* (which calls into the corresponding exec* C functions) “swaps out” the current process entirely, so the current process (which is running pytest) just vanishes in thin air when you call that. This is done at the OS level, thus not possible to verify with pytest. The best thing you can do is to mock it out and verify the arguments.

[itsayellow]

Ok, so after now firing up my Windows VM, I see that os.exec* seems to fail with Segmentation fault on Windows. (EDIT it crashes inside of MSYS2 but works inside of cmd.exe 😦 )

I couldn't find any good documentation on why, however. ox.exec* lists availability on Windows. @uranusjr, why do you need to fall back to subprocess for Windows?

Do we just need to use our original code for Windows? Or is there an advantage to how pipenv does things with subprocess.Popen(), etc.?

[itsayellow]

There is one pytest test that still uses os.exec because it doesn't need to be mocked (test_run_ensure_null_pythonpath in test_run.py), and it fails only on Windows by returning exit code 3221225477, which supposedly means "STATUS_ACCESS_VIOLATION, which means you tried to access (read, write or execute) invalid memory. "

Running the same pipx run command by hand in the Windows shell does work and returns the expected string.

I suppose this could be an artifact of pytest?? I can probably mock this test as well but it would make me feel better if all this Windows process stuff just worked...

[uranusjr]

I couldn't find any good documentation on why, however. ox.exec* lists availability on Windows. @uranusjr, why do you need to fall back to subprocess for Windows?

Do we just need to use our original code for Windows? Or is there an advantage to how pipenv does things with subprocess.Popen(), etc.?

Windows and Unix-like systems implement exec* stuff differently. On POSIX, the process is first forked, and then the new program is loaded into the (forked) process. Windows does not have fork, so the implementation basically starts a new process in the background, and exits the current process. This is fine for Windows (the subsystem, not the operating system) applications, but has some unexpected (to people familiar with the fork-based implementation) behaviours when used in the console subsystem. You can try this in cmd.exe:

C:\>py -c "import os, sys; os.execl(sys.executable, 'python', '-V')"

C:\>Python 3.9.0

You can see a prompt being displayed before the version information, because the process launched with os.execl is in the background, and you get dropped back to the shell when the former process exits. This causes more problems when you need to interact with the launched process:

C:\>py -c "import os, sys; os.execl(sys.executable, 'python', '-q')"

C:\>>>> import sys
'import' is not recognized as an internal or external command,
operable program or batch file.

Since the new process is in the background, and the user is dropped back to the shell, all inputs now goes to the shell, not the launched process.

Rule of thumb: Don’t use the exec* family on Windows in a console application. It never does what you want.

[itsayellow]

Rule of thumb: Don’t use the exec* family on Windows in a console application. It never does what you want.

😆

I think I found that out experimentally. The current version of the code I'm writing just drops back to subprocess for Windows, which seems what you recommend and pipenv does.

Thanks for the thorough explanation!