Hi everyone,

here is v2 of this patch since there were no additional feedback and I still
think that this patch is an improvement to the status quo.

v1 -> v2: fix return type and formatting

Copy of the original cover letter:
> In multiple plugins I manage long running processes using vis:communicate.
> This is the case in vis-lspc and my vis-inotify plugin, which I use to
> atomically reload the theme if the theme file on disk changed.
> 
> Both plugins are leaking their processes, because they are not terminated
> when vis exits and are simply passed on to init.
> 
> While vis is running:
> 	$ ps axo cmd,pid,ppid | grep ino
> 	inotifywait -P /home/muhq/.  115972  115948
> 
> After vis was closed:
> 	$ ps axo cmd,pid,ppid | grep ino
> 	inotifywait -P /home/muhq/.  115972       1
> 
> Even a graceful shutdown by subscribing to the QUIT event does not work because
> the subprocesses are only checked and killed during the main loop in vis_run.
> 
> Those patches fix this issue by checking the lifetime and potentially
> killing processes one last time after the QUIT event was emitted.
> This allows plugin developers to subscribe to this event and shutdown their long
> running processes without leakage.
> 
> See: The vis-inotify [1] and vis-lspc [2] commits
> 
> [1]: https://git.muhq.space/vis-inotify/commit/?id=61f79bb3046e4b548f2a1798abccb7b53e284516
> [2]: https://git.muhq.space/vis-lspc/commit/?id=62e6a847a85e25037a4a7fbcd4af697928d5ecfc

--
Flo

The separation between reading from a subprocess and handling its
life time will be useful for future changes.
---
 vis-subprocess.c | 54 ++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/vis-subprocess.c b/vis-subprocess.c
index 39282e47..31a14796 100644
--- a/vis-subprocess.c
+++ b/vis-subprocess.c
@@ -176,6 +176,36 @@ static void read_and_fire(Vis* vis, int fd, const char *name, ResponseType rtype
 	}
 }
 
+/**
+ * Checks if a subprocess is dead or needs to be killed then raises an event
+ * or kills it if necessary.
+ * @param current the process to wait for or kill
+ * @return true if the process is dead
+ */
+static bool wait_or_kill_process(Vis *vis, Process *current) {
+	int status;
+	pid_t wpid = waitpid(current->pid, &status, WNOHANG);
+	if (wpid == -1) {
+		vis_message_show(vis, strerror(errno));
+	} else if (wpid == current->pid) {
+		goto just_destroy;
+	} else if (!*(current->invalidator)) {
+		goto kill_and_destroy;
+	}
+	return false;
+
+kill_and_destroy:
+	kill(current->pid, SIGTERM);
+	waitpid(current->pid, &status, 0);
+just_destroy:
+	if (WIFSIGNALED(status)) {
+		vis_lua_process_response(vis, current->name, NULL, WTERMSIG(status), SIGNAL);
+	} else {
+		vis_lua_process_response(vis, current->name, NULL, WEXITSTATUS(status), EXIT);
+	}
+	return true;
+}
+
 /**
  * Checks if `readfds` contains file descriptors of subprocesses from
  * the pool. If so, it reads their data and fires corresponding events.
@@ -192,27 +222,11 @@ void vis_process_tick(Vis *vis, fd_set *readfds) {
 		if (current->errfd != -1 && FD_ISSET(current->errfd, readfds)) {
 			read_and_fire(vis, current->errfd, current->name, STDERR);
 		}
-		int status;
-		pid_t wpid = waitpid(current->pid, &status, WNOHANG);
-		if (wpid == -1)	{
-			vis_message_show(vis, strerror(errno));
-		} else if (wpid == current->pid) {
-			goto just_destroy;
-		} else if (!*(current->invalidator)) {
-			goto kill_and_destroy;
-		}
-		pointer = &current->next;
-		continue;
-kill_and_destroy:
-		kill(current->pid, SIGTERM);
-		waitpid(current->pid, &status, 0);
-just_destroy:
-		if (WIFSIGNALED(status)) {
-			vis_lua_process_response(vis, current->name, NULL, WTERMSIG(status), SIGNAL);
+		if (!wait_or_kill_process(vis, current)) {
+			pointer = &current->next;
 		} else {
-			vis_lua_process_response(vis, current->name, NULL, WEXITSTATUS(status), EXIT);
+			/* update our iteration pointer */
+			*pointer = destroy_process(current);
 		}
-		/* update our iteration pointer */
-		*pointer = destroy_process(current);
 	}
 }
-- 
2.47.1

Currently there is now way for long running subprocesses like language
servers to gracefully shutdown.
When reacting to the QUIT event and invalidating the process handle
the subprocess will never be killed and destroyed because the
subprocesses are only checked during vis_run.

Collecting and killing subprocesses with invalid handles after the
QUIT event allows graceful shutdown.
---
 vis-subprocess.c | 16 ++++++++++++++++
 vis-subprocess.h |  1 +
 vis.c            |  1 +
 3 files changed, 18 insertions(+)

diff --git a/vis-subprocess.c b/vis-subprocess.c
index 31a14796..e17d24f4 100644
--- a/vis-subprocess.c
+++ b/vis-subprocess.c
@@ -230,3 +230,19 @@ void vis_process_tick(Vis *vis, fd_set *readfds) {
 		}
 	}
 }
+
+/**
+ * Checks if each subprocess from the pool is dead or needs to be
+ * killed then raises an event or kills it if necessary.
+ */
+void vis_process_waitall(Vis *vis) {
+	for (Process **pointer = &process_pool; *pointer; ) {
+		Process *current = *pointer;
+		if (!wait_or_kill_process(vis, current)) {
+			pointer = &current->next;
+		} else {
+			/* update our iteration pointer */
+			*pointer = destroy_process(current);
+		}
+	}
+}
diff --git a/vis-subprocess.h b/vis-subprocess.h
index 2e4c2220..79a043e9 100644
--- a/vis-subprocess.h
+++ b/vis-subprocess.h
@@ -27,4 +27,5 @@ Process *vis_process_communicate(Vis *, const char *command, const char *name,
                                  Invalidator **invalidator);
 int vis_process_before_tick(fd_set *);
 void vis_process_tick(Vis *, fd_set *);
+void vis_process_waitall(Vis *);
 #endif
diff --git a/vis.c b/vis.c
index 10c3df57..d1c0dab0 100644
--- a/vis.c
+++ b/vis.c
@@ -623,6 +623,7 @@ void vis_free(Vis *vis) {
 	while (vis->windows)
 		vis_window_close(vis->windows);
 	vis_event_emit(vis, VIS_EVENT_QUIT);
+	vis_process_waitall(vis);
 	file_free(vis, vis->command_file);
 	file_free(vis, vis->search_file);
 	file_free(vis, vis->error_file);
-- 
2.47.1

Florian Fischer <florian.fischer@muhq.space> wrote:
> Hi everyone,
> 
> here is v2 of this patch since there were no additional feedback and I still
> think that this patch is an improvement to the status quo.
> 
> v1 -> v2: fix return type and formatting
> 
> Copy of the original cover letter:
> > In multiple plugins I manage long running processes using vis:communicate.
> > This is the case in vis-lspc and my vis-inotify plugin, which I use to
> > atomically reload the theme if the theme file on disk changed.
> > 
> > Both plugins are leaking their processes, because they are not terminated
> > when vis exits and are simply passed on to init.
> > 
> > While vis is running:
> > 	$ ps axo cmd,pid,ppid | grep ino
> > 	inotifywait -P /home/muhq/.  115972  115948
> > 
> > After vis was closed:
> > 	$ ps axo cmd,pid,ppid | grep ino
> > 	inotifywait -P /home/muhq/.  115972       1
> > 
> > Even a graceful shutdown by subscribing to the QUIT event does not work because
> > the subprocesses are only checked and killed during the main loop in vis_run.
> > 
> > Those patches fix this issue by checking the lifetime and potentially
> > killing processes one last time after the QUIT event was emitted.
> > This allows plugin developers to subscribe to this event and shutdown their long
> > running processes without leakage.
> > 
> > See: The vis-inotify [1] and vis-lspc [2] commits
> > 
> > [1]: https://git.muhq.space/vis-inotify/commit/?id=61f79bb3046e4b548f2a1798abccb7b53e284516
> > [2]: https://git.muhq.space/vis-lspc/commit/?id=62e6a847a85e25037a4a7fbcd4af697928d5ecfc
> 
> --
> Flo

Hi Florian,

I'll give you the benefit of the doubt since you wrote both those
plugins and are probably understanding this better than I am. I do
agree that at least notifying the plugins that the editor is
exiting is a good idea even if I don't personally support wasting
the users time by calling free and close on program exit.

Applied!

-- 
https://rnpnr.xyz/
GPG Fingerprint: B8F0 CF4C B6E9 415C 1B27 A8C4 C8D2 F782 86DF 2DC5