Remove the double forking in start_application #5510
Conversation
Having just a single fork is beneficial, as it preserves the approprate parent information for the children of i3, which is useful in some scenarious e.g. when a child wants to do something on the wm's exit (possible via `prctl(PR_SET_PDEATHSIG, ...)`). Moreover, this is a zero-cost benefit: i3 is already using libev with the default loop, which automatically reaps all the zombie children even when there is no corresponding event set. Resolves i3#5506
kolayne
force-pushed
the
remove_double_fork
branch
from
8b521a1
to
754c4f2
Compare
|
So, this would be a backwards-incompatible change (even if the new behavior is an improvement). I wonder how this is going to affect users and if there are usecases where keeping programs alive after i3's exit is beneficial. Also, will changing this mean programs get killed after a restart? Because that would absolutely break users' expectations of i3 and can cause lost work. |
|
Programs won't get killed neither on restart nor on i3 exit, unless the child process explicitly requests that via The restart should not involve this at all, as i3 |
TL;DRis in the next comment ClarificationTo reiterate: the double And the benefit of the patch is that the valid parent information is preserved. Children can rely on it however they want, for example, in my particular use case, automatically die when the wm exits (but the patch does not enforce this behavior in any way). BehaviorFor the tests I used simple programs of 3 types:
Here are their source codes careless.c#include <stdio.h>
#include <unistd.h>
int main() {
for(int i = 0; i < 6; ++i) {
puts("I don't care who my parent is, I am running");
fflush(stdout);
sleep(2);
}
puts("Exiting gracefully");
}parent_watcher.c#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
void print_cmd_name(pid_t pid) {
char buf[512] = "/proc/";
sprintf(buf + 6, "%ld", pid);
size_t pos = strlen(buf);
memcpy(buf + pos, "/comm", 6);
FILE *f = fopen(buf, "r");
if(!f) {
puts("couldn't get their executable name");
return;
}
fgets(buf, 512, f);
printf("which is %s", buf);
}
int main() {
for(int i = 0; i < 6; ++i) {
pid_t ppid = getppid();
printf("My current parent is %ld, ", ppid);
print_cmd_name(ppid);
fflush(stdout);
sleep(2);
}
puts("Exiting gracefully");
}pdeath_waiter.c#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/prctl.h>
#include <errno.h>
#include <assert.h>
void handle_hup(int _signal) {
puts("Got a SIGHUP! Proceeding with the execution");
}
int main() {
prctl(PR_SET_PDEATHSIG, SIGHUP, 0, 0, 0);
assert(!errno);
struct sigaction sa;
sa.sa_handler = handle_hup;
sigemptyset(&sa.sa_mask);
sa.sa_flags = 0;
sigaction(SIGHUP, &sa, NULL);
assert(!errno);
for(int i = 0; i < 6; ++i) {
puts("I'm waiting for a notification");
fflush(stdout);
sleep(2);
}
puts("Exiting gracefully");
}I ran each of them on both original and the patched versions and checked their behavior on i3's restart and exit (the lines starting with Output with upstream i3careless does not care: it keeps running no matter what. parent_watcher gets reparented immediately. On my system there is no subreapers, so it gets adopted by PID1: Just like parent_watcher, pdeath_waiter becomes an orphan very early (in fact, so early that it doesn't have enough time to call Output with patched i3For careless nothing changes, the behavior is exactly the same: parent_watcher now correctly identifies pdeath_waiter now can do arbitrary actions when i3 exits! |
TL;DR
|
|
@orestisfl, is there any action needed from my side? Should I explicitly request someone's review or just wait? |
|
It seems that the double fork logic was added in f4f4d78. So, @stapelberg, I would appreciate your comments |
No, that commit only moves the code around. It was originally added in 4589c26, way before we even used libev. The change seems reasonable to me, or at least reasonable enough to try it out :) |
Having just a single fork is beneficial, as it preserves the approprate parent information for the children of i3, which is useful in some scenarious e.g. when a child wants to do something on the wm's exit (possible via `prctl(PR_SET_PDEATHSIG, ...)`). Moreover, this is a zero-cost benefit: i3 is already using libev with the default loop, which automatically reaps all the zombie children even when there is no corresponding event set. Resolves i3#5506
Having just a single fork is beneficial, as it preserves the approprate parent information for the children of i3, which is useful in some scenarious e.g. when a child wants to do something on the wm's exit (possible via
prctl(PR_SET_PDEATHSIG, ...)).Moreover, this is a zero-cost benefit: i3 is already using libev with the default loop, which automatically reaps all the zombie children even when there is no corresponding event set.
Quote from the
ev(3)man page (emphasis mine):Resolves #5506