-
Notifications
You must be signed in to change notification settings - Fork 78
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
BackgroundCommand signals don't propagate through some shell setups #645
Comments
After discussing offline with @mrkajetanp , it seems that a solution might be to change the PID detection code:
In order to detect what is a child PID and what is a wrapper layer, we can try to manipulate the |
Also it's important to note that the signal propagation is not limited to
So this problem needs to be solved regardless of what In order to make |
Another avenue would be to insert an extra bit of shell script before the user-provided command:
Then devlib can unblock the loop every time it wants an up-to-date list of PIDs. @cloehle raised the issue of the user snippet waiting for all its children (which would wait forever if that includes the function we spawned). This can maybe be worked around by wrapping the user snippet in an |
After some thinking, I feel like the best solution is probably the most explicit one: the user-provided shell snippet should have access to a shell function that allows declaring what is the "PID of that snippet". This would then be collected by the Python part and signals would be delivered to that specific task. Code that wouldn't use that helper would be limited to sending SIGKILL to the command (as it de facto is today): # The process spawned by devlib-run would have its PID made available to the Python API
target.background('if [ foo ]; then devlib-run mycommand; else foobar; fi') This allows arbitrarily complex shell snippet to still be handled in the most sensible way by letting the user declare the "main event" in the background command. This is particularly important for signals like SIGUSR1, that the main command might handle in a special way, but may result in terminating any other helper commands that happens to be executed at that time in the snippet if the signal was delivered to all processes. In terms of implementation, we can easily pre-pend something like that to the user-provided shell command:
@marcbonnici what do you think about this solution ? |
Started working on it |
On some platforms (e.g. Android with
su
coming from Magisk) there seems to bean issue where BackgroundCommand.send_signal() does not propagate through the
outer
sh
layers to the inner process. The same implementation that works okaywhether with
adb root
or just withsu
coming from Android breaks on theMagisk setup. Manually finding the PID of the inner process and sending the
signal to it on the Magisk setup works around the problem which strongly
indicates it's a signal propagation issue.
The process hierarchy ends up looking as follows:
Other platforms might potentially be affected as well.
The text was updated successfully, but these errors were encountered: