Openurlonclick leaves zombies behind

[veltza]

I noticed that when a web browser is open, the openurlonclick patch leaves zombie processes behind.

The reason is that the patch uses the posix_spawnp function to start the xdg-open command, and since the patch doesn't wait for the command to finish, the command can turn into a zombie process.

The simplest solution I know is to replace the function with the double-fork method, which creates orphaned processes that the init system takes care of.

Edit: I may have changed my mind. Maybe we should rewrite the sigchld function to reap all child processes, since the externalpipe patch can also produce zombies.

[bakkeby]

Besides double forking one solution would be to add a call to waitpid to read the exit status of the child process.

diff --git a/patch/openurlonclick.c b/patch/openurlonclick.c
index 54b1a02..0e66e43 100644
--- a/patch/openurlonclick.c
+++ b/patch/openurlonclick.c
@@ -223,8 +223,10 @@ openUrlOnClick(int col, int row, char* url_opener)
        char *url = detecturl(col, row, 1);
        if (url) {
                extern char **environ;
-               pid_t junk;
+               pid_t pid;
                char *argv[] = { url_opener, url, NULL };
-               posix_spawnp(&junk, argv[0], NULL, NULL, argv, environ);
+               posix_spawnp(&pid, argv[0], NULL, NULL, argv, environ);
+               /* Wait for the child process to finish */
+               waitpid(pid, NULL, 0);
        }
 }
diff --git a/patch/openurlonclick.h b/patch/openurlonclick.h
index 5aa09de..f3011bf 100644
--- a/patch/openurlonclick.h
+++ b/patch/openurlonclick.h
@@ -1,4 +1,5 @@
 #include <spawn.h>
+#include <sys/wait.h>

 static inline void restoremousecursor(void) {
        if (!(win.mode & MODE_MOUSE) && xw.pointerisvisible)

That is assuming of course that the spawned process will end quickly (and not block the terminal while it is running).

Another option is to set the signal handler to ignore the response from children.

diff --git a/patch/openurlonclick.c b/patch/openurlonclick.c
index 54b1a02..baeb021 100644
--- a/patch/openurlonclick.c
+++ b/patch/openurlonclick.c
@@ -225,6 +225,7 @@ openUrlOnClick(int col, int row, char* url_opener)
                extern char **environ;
                pid_t junk;
                char *argv[] = { url_opener, url, NULL };
+               signal(SIGCHLD, SIG_IGN);
                posix_spawnp(&junk, argv[0], NULL, NULL, argv, environ);
        }
 }

This seems fine at first glance, but can potentially have consequences for other spawned processes.

A third option is to pass the POSIX_SPAWN_SETSID attribute flag when calling posix_spawnp.

diff --git a/patch/openurlonclick.c b/patch/openurlonclick.c
index 54b1a02..a2f665c 100644
--- a/patch/openurlonclick.c
+++ b/patch/openurlonclick.c
@@ -225,6 +225,9 @@ openUrlOnClick(int col, int row, char* url_opener)
                extern char **environ;
                pid_t junk;
                char *argv[] = { url_opener, url, NULL };
-               posix_spawnp(&junk, argv[0], NULL, NULL, argv, environ);
+               posix_spawnattr_t attr;
+               posix_spawnattr_init(&attr);
+               posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSID);
+               posix_spawnp(&junk, argv[0], NULL, &attr, argv, environ);
        }
 }

The latter is perhaps less ideal because the solution is GNU specific and will need -D_GNU_SOURCE=1 to be added to the STCPPFLAGS in config.mk.

[veltza]

Besides double forking one solution would be to add a call to waitpid to read the exit status of the child process.
That is assuming of course that the spawned process will end quickly (and not block the terminal while it is running).

The xdg-open command only finishes quickly when a web browser is already open. So we can't wait for it.

Another option is to set the signal handler to ignore the response from children.
This seems fine at first glance, but can potentially have consequences for other spawned processes.

Exactly. There is already a sigchld handler that was originally designed to only handle the termination of the shell process. We could rewrite it to handle zombie processes as well. In fact, the externapipe patch tries to do that but it works in the wrong way. First it installs the sigchld handler with sigaction() and then tries to reinstall it with sigchld(), which is unnecessary since sigaction() doesn't require that. And what's worse, sometimes it reinstalls it correctly and sometimes it doesn't, which breaks the handler and also leads to the zombie issue.

So the suggestion is that we rewrite the sigchld handler like this:

void
sigchld(int a)
{
	int stat;
	pid_t p;

	while ((p = waitpid(-1, &stat, WNOHANG)) > 0) {
		if (p == pid) {
			if (WIFEXITED(stat) && WEXITSTATUS(stat))
				die("child exited with status %d\n", WEXITSTATUS(stat));
			else if (WIFSIGNALED(stat))
				die("child terminated due to signal %d\n", WTERMSIG(stat));
			_exit(0);
		}
	}
}

It would still handle the termination of the shell process and, in addition, handle the zombie processes as well.

The handler would then be installed almost the same as in the externalpipe patch:

	memset(&sa, 0, sizeof(sa));
	sigemptyset(&sa.sa_mask);
	sa.sa_flags = SA_RESTART;
	sa.sa_handler = sigchld;
	sigaction(SIGCHLD, &sa, NULL);

And finally, wait() and waitpid() calls should be removed from the newterm and rightclicktoplumb patches because the handler takes care of them.

That should fix the zombie processes that current patches and any future patches may produce. But since I have little experience with the signal handling, a second opinion is always welcome.

[UtkarshVerma]

Have we reached a solution for this? I wanted to create a PR for the openurlonclick patch. #157

If this is sorted out, we can have both the changes incorporated.

[veltza]

Have we reached a solution for this? I wanted to create a PR for the openurlonclick patch. #157

I've been using the above solution in my own fork for a few weeks now and it's working fine. But I still don't know what kind of solution bakkeby prefers.

If this is sorted out, we can have both the changes incorporated.

I would keep them separate.

[bakkeby]

The second option to just add signal(SIGCHLD, SIG_IGN); is of course the least invasive here.

I don't mind a bit of refactoring here and the proposed sigchld function looks perfectly fine to me. I am wondering a bit about the case of combining the externalpipe and the externalpipein patches. Possibly it may make sense to use a separate function for this.

My main problem here is that I do not have a lot of capacity to test all of the affected patches, but the principle of veltza's proposal look fine to me.

[veltza]

I am wondering a bit about the case of combining the externalpipe and the externalpipein patches.

The above solution also works with the externalpipe, but since I don't have any real scripts to test the externalpipein patch, I can't guarantee that.

But if you prefer an isolated solution, I can also send a pull request that just replaces posix_spawnp() with the double-fork method in the openurlonclick patch?

That solution only fixes the openurlonclick, but the externalpipe can still spawn zombies because there is a bug in its code. Would you like me to open a new issue about it if someone wants to work on that bug?

[bakkeby]

There are not that many patches involved so I'd say let's keep it in this ticket. Let me start a branch and pull request then we can see if there are any holes.

[bakkeby]

I tested the externalpipein patch using the example command in the default config (setbgcolorcmd) and it seems to still work after the above changes.

Let me know if I have missed something.