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

Bootloader: forward signals to bundled application #3515

Merged
merged 5 commits into from Aug 25, 2018

Conversation

Projects
None yet
2 participants
@brettdh
Contributor

brettdh commented May 15, 2018

Forward more signals

All the signals, to be precise. (Well, all the signals that can be handled.)

If the caller of pyinstaller specifies the --bootloader-ignore-signals CLI flag, all signals will be ignored instead. This is useful if e.g. a supervisor process signals the bootloader's process group; otherwise, the child process would be signaled twice.

Note: docs forthcoming for the new flag (and perhaps this overall behavior), pending feedback on the overall approach.

New functional test

This test checks whether signals sent to an pyinstaller-built executable can be handled
using signal.signal(...) in the Python code.

Summary of my manual testing so far (before this test):

  • Linux
    • Fails in --onedir and --onefile modes.
  • macOS
    • Fails only in --onefile mode
  • Windows
    • Not tested - and I suspect I should skip this test on Windows, since signals aren't the same there.
    • (Also, I don't really have any idea how signals, or whatever semi-equivalent, work on Windows.)

At present, I'm assuming that an application using pyinstaller could reasonably want to handle any handleable signal (i.e. not SIGKILL or SIGSTOP). If there are any exceptions to this, or if this needs to be handled more selectively for reasons I'm not aware of, I'd be glad to hear about it.

Closes #3514.

@brettdh brettdh changed the title from WIP: Bootloader: forward signals to to WIP: Bootloader: forward signals to bundled application May 15, 2018

@brettdh brettdh force-pushed the brettdh:develop branch from 737de93 to 28ba8df May 16, 2018

@brettdh brettdh changed the title from WIP: Bootloader: forward signals to bundled application to Bootloader: forward signals to bundled application May 16, 2018

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 16, 2018

CI build is looking good so far. I'm optimistically declaring this ready for review.

@brettdh brettdh force-pushed the brettdh:develop branch from 28ba8df to 5e37595 May 17, 2018

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 17, 2018

Added @skipif_win for the signal tests, 'cause I have no idea how to handle the Windows case, and I kinda doubt that it's common to do much with signals and PyInstaller on Windows.

Other Appveyor errors seem unrelated to this PR.

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 24, 2018

There's a subtle problem with this PR: messing with SIGCHLD makes the boot loader unable to wait() for the child and capture its exit status. Since the exit code of wait() is not checked, this makes it appear that the child always exits 0.

I have a fix in flight; will hopefully push soon, along with a related piece of functionality that I think is required here (the ability to ignore signals in the boot loader if desired).

@htgoebel

This comment has been minimized.

Member

htgoebel commented May 26, 2018

In the meanwhile: thanks for working on this :-) I'm expecting our updates.

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 26, 2018

I'm currently stuck on this issue: I have a test that breaks with py.test -n4 (or -n1) but succeeds without it. I'm wondering if pytest-xdist or execnet is getting thrown off by the boot loader ignoring all signals. Does that happen to raise any red flags as to what might be going wrong?

I've just pushed the branch in its current broken state to show what I mean. Suggestions welcome.

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 26, 2018

If the failure in Travis looks like my local testing, it'll show that the call to PI_Py_Finalize appears to crash the child, as there's no output after that. Also, the parent exits with a negative integer value, indicating that the signal was not ignored as it should have been.

But, I've also been seeing strange results where debug prints that I insert in the parent don't show up in the output, even though the child process is still forked and runs the test. So there might be something wonky with my test environment. I'm very curious to see what the same test in Travis looks like.

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 26, 2018

...and of course, since I wasn't thinking and pushed commits to another PR before this one, it's going to be a while before this test runs. 🤦‍♂️

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 26, 2018

Lol and as I write that, the build kicked off after all. Pleasantly surprised.

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 26, 2018

Ok, so - the failures I see locally don’t show up in Travis. I need to fix the SIGCLD case (treat it like SIGCHLD), but otherwise I guess this is almost done.

Figuring out the issue with my local test env can be a part of #3519.

@brettdh

This comment has been minimized.

Contributor

brettdh commented May 26, 2018

@htgoebel this should be good for review; I just pushed the SIGCLD fix.

@brettdh brettdh force-pushed the brettdh:develop branch from ca85ca7 to 6b0e74e Jun 5, 2018

@brettdh

This comment has been minimized.

Contributor

brettdh commented Jun 5, 2018

Rebased, resolved conflicts, squashed a bit.

@brettdh

This comment has been minimized.

Contributor

brettdh commented Jun 14, 2018

@htgoebel any news on this? Anything else you need me to do?

@htgoebel htgoebel added the bootloader label Jun 27, 2018

@htgoebel htgoebel self-requested a review Jun 27, 2018

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Jun 27, 2018

@htgoebel

Thanks a lot for this pull-request. Sorry for not answering earlier, I'm busy with my day-job.

This looks good for me and I like the test-suite you've created. Nevertheless I added some comments, most of which are nit-picking to keep the code maintainable for another 10 years.

@@ -56,7 +56,7 @@ def __init__(self, *tocs, **kwargs):
ModuleGraph.
kwargs
Possible keywork arguments:
Possible keyword arguments:

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

This is unrelated to this change and should go into a commit of its own. Same for typo fixed below. I'd even abstain from fixing then, since it is still understandable.

target = child
eprint('signalling {{}} ({{}})'.format(target.name(), target.pid))

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

By using %s interpolation, this becomes easer to read. Same for all other cases below. and above.

Also: Why not passing several args to eprint:
``
eprint('signaling', target.name(), '(%s)' % target.pid)

This is why print is designed as it is :-) And you can get rid of the formatting at all in many cases.
print('Bootloader did not fork; test is invalid')
sys.exit(0)
target = child

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

This trailing target = child looks lost. What about this structure:

if parent.name ...
   ...
elif ignore:
 ...
else:
  ...
if signame in ['SIGCHLD', 'SIGCLD']:
pytest.xfail(
'Messing with {} interferes with bootloader'.format(signame)
)

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

Please add a short comment. I assume these signals are not filtered out when building the signals list to spot whether the bootloader actually does not forward these to the child?!

VS("LOADER: Restoring signal handlers\n");
for (signum = 0; signum < num_signals; ++signum) {
if (handlers[signum] != SIG_ERR) {
signal(signum, handlers[signum]);

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

Not setting the signal back to SIG_DFL (as the old code did) is worth a commit of its own. Thus when reading the commit messages one can easily spot this relevant change.

This comment has been minimized.

@brettdh

brettdh Jul 10, 2018

Contributor

@htgoebel It's pretty insignificant, IMO, particularly since the bootloader (parent) is about to exit after this. I'd be more inclined to switch back to using SIG_DFL.


#ifndef sighandler_t
typedef void (*sighandler_t)(int);
#endif

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

Please move this near the top of the file, where other type-defs are.

{
kill(child_pid, signal);
VS("LOADER: Forwarding signal %d to child pid %d\n", signum, child_pid);
kill(child_pid, signum);

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

Renaming parameters is a non-functional change, please revert this. This will increase the patch-sice and the time to review and understand what this change is doing.

This comment has been minimized.

@brettdh

brettdh Jul 10, 2018

Contributor

The previous name, signal, was shadowing a function name. Like you, I'm interested in the long-term maintainability of the code I write, so I fixed this issue. I've already split this tiny function into two handlers; even calling this a rename is a stretch.

@@ -850,21 +855,44 @@ set_systemd_env()
pid_t child_pid = 0;

static void
_signal_handler(int signal)
_ignore_signal(int signum)

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

Rename to something telling this is a handler, too. E.g. ignoring_signal_handler. Also a comment would be nice.

This comment has been minimized.

@brettdh

brettdh Jul 10, 2018

Contributor

What would the comment say that isn't already obvious from the function name and log message?

@@ -553,7 +553,7 @@ pyi_arch_status_free_memory(ARCHIVE_STATUS *archive_status)
* for freeing it.
*/
char *
pyi_arch_get_option(ARCHIVE_STATUS * status, char * optname)
pyi_arch_get_option(const ARCHIVE_STATUS * status, char * optname)

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

These changes (adding "const") are unrelated to this pull-request and should go into a commit of its own.

This comment has been minimized.

@brettdh

brettdh Jul 10, 2018

Contributor

In fact, these were required, because I added an argument of type const ARCHIVE_STATUS * to pyi_utils_create_child below.

I can either make these existing methods const-correct or I can make the new argument const-incorrect. I chose the option that I felt was a net improvement. Please let me know if you disagree.

"Useful in situations where e.g. a supervisor "
"process signals both the bootloader and child "
"(e.g. via a process group) to avoid signalling "
"the child twice."))

This comment has been minimized.

@htgoebel

htgoebel Jul 6, 2018

Member

What do you think about moving this below --noupx, or even into the "Rarely used special options:" section?

This comment has been minimized.

@brettdh

brettdh Jul 10, 2018

Contributor

Sure; though I'm not really sure how common this use case is, I lean towards the latter.

@brettdh brettdh force-pushed the brettdh:develop branch from 6b0e74e to 2ddb80d Jul 10, 2018

@brettdh

This comment has been minimized.

Contributor

brettdh commented Jul 10, 2018

Addressed feedback and pushed new version.

htgoebel added some commits Aug 16, 2018

@htgoebel htgoebel merged commit a1ecdfb into pyinstaller:develop Aug 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment