[Lua API] Non-blocking subprocess runner

[xomachine]

To improve the plugins API it would be nice to have a mechanism to run and communicate with a subprocess without whole editor hang. E. g. such a mechanism needed to make better implementation of my Nim language plugin. The plugin spawns a process which caries all information about source code and suggests/checks/gives info in interactive mode using stdin/stdout to communicate with caller. At the moment, the communication problem is solved by kicking vis from the shell using window resize signal and some other dirty hacks to make pipes non-blocking.

Attached implementation provides the vis.communicate function which starts a process in the shell and returns a file handle to write data to its stdin. If Lua code closes the file handle, the process is being killed. When the process writes anything to stdout or stderr, quits or gets signal - the PROCESS_RESPONCE event is being fired with corresponding arguments. The implementation allows multiple subprocesses running in the same time. They can be distinguished by the name argument passed to the vis.communicate function and to the event handler when the event fires. The responsibility of the name uniqueness is laid on the users code.

[erf]

This sounds promising. Is this similar to how vim8 jobs / channels works?

[xomachine]

I am not quite familiar with vim channels, but at quick glance it looks like there are some similarities indeed.

All data is being sent to a process asyncroniously without waiting for response like it is in vim, although instead of passing a callback for handling the process answers my solution utilizes the event system already implemented in vis.

[mcepl]

Would it be possible to rebase this patch on the top of the current master, please? This is making testing rather difficult.

[xomachine]

Would it be possible to rebase this patch on the top of the current master, please? This is making testing rather difficult.

Done. Hopefully I did not break anything.

[mcepl]

@xomachine see the linked issue report: this patch is suspected to be the cause of the crash.

[euclaise]

Any updates on this? My understanding is that this is needed to implement LSP, and losing LSP without manually patching is a big point for many that may prevent switching to vis (e.g. it is the sole reason why I haven't switched from nvim).

[mcepl]

Hmm, when I pulled in also new tests, I got this one test failing on the aarch64 architecture (not on Intels, there it works just fine):

[  133s] make: Leaving directory '/home/abuild/rpmbuild/BUILD/vis/test/core'
[  133s] + /usr/bin/make -O -j4 V=1 VERBOSE=1 -C test/lua
[  134s] make: Entering directory '/home/abuild/rpmbuild/BUILD/vis/test/lua'
[  134s] vis v0.8-git +curses +lua +tre +acl +selinux
[  134s] cursor                        OK
[  134s] file-empty                    OK
[  134s] lines                         OK
[  134s] map-basic                     OK
[  134s] subprocess                    ./test.sh: line 34:  2489 Killed                  $VIS "$t.in" < infifo 2> /dev/null > "$t.busted"
[  134s] FAIL
[  134s] +++
[  134s] 3 successes / 0 failures / 0 errors / 0 pending : 0.021478 seconds
[  134s] The following events did not happened for process	inputtest
[  134s] 1	: 	STDERR	 - 	testdata
[  134s] 
[  134s] 1: 	STDERR	 - (string)	testdata
[  134s] 
[  134s] 2	: 	EXIT	 - 	0
[  134s] 2: 	EXIT	 - (number)	0
[  134s] Tests ok 4/5

Complete build log

[xomachine]

That's weird. My only guess is different behavior of the read command on aarch64. Vis was able to get the EXIT event in previous test, but did not get it in the last one, where the read command was involved. Therefore the child process did not finish and still waiting for the input at stdin. So that could be the reason why none of events fired.

[mcepl]

I am thinking how to persuade people in charge here (@martanne , @rnpnr , @ninewise ) to review this pull request, and I have to admit that when I tried myself, I don’t find the task easy. Would it be possible, please, to reorganize the patches to be topical ones (something like this or like this), i.e., each change should contain all changes in one area? Commits like “Fixes of …” or “Correct implementation of …” just make reviewer’s life more complicated and less willing to help.

[rnpnr]

That would definitely be appreciated. There seems to be a lot of commits that should be squashed together at a minimum.

I do want to get around to reviewing this one since I think it has the potential to be very useful but I've been trying to work through some less complicated things first.

[xomachine]

I guess the best thing I can do with this history is squashing everything into one commit and rewriting the commit message. There are no logical changes in this patch set, it is just a trial-and-error story of implementing the feature.

[mcepl]

Well, if I do git diff origin/master 91bb1dd I get

diff --git a/Makefile b/Makefile
index c862643..4c473bf 100644
--- a/Makefile
+++ b/Makefile
@@ -26,6 +26,7 @@ SRC = array.c \
 	vis-prompt.c \
 	vis-registers.c \
 	vis-text-objects.c \
+	vis-subprocess.c \
 	$(REGEX_SRC)
 
 ELF = vis vis-menu vis-digraph
diff --git a/lua/vis.lua b/lua/vis.lua
index 39649c1..f2f9421 100644
--- a/lua/vis.lua
+++ b/lua/vis.lua
@@ -152,6 +152,7 @@ local events = {
 	WIN_OPEN = "Event::WIN_OPEN", -- see @{win_open}
 	WIN_STATUS = "Event::WIN_STATUS", -- see @{win_status}
 	TERM_CSI = "Event::TERM_CSI", -- see @{term_csi}
+	PROCESS_RESPONSE = "Event::PROCESS_RESPONSE", -- see @{process_response}
 }
 
 events.file_close = function(...) events.emit(events.FILE_CLOSE, ...) end
@@ -167,6 +168,7 @@ events.win_highlight = function(...) events.emit(events.WIN_HIGHLIGHT, ...) end
 events.win_open = function(...) events.emit(events.WIN_OPEN, ...) end
 events.win_status = function(...) events.emit(events.WIN_STATUS, ...) end
 events.term_csi = function(...) events.emit(events.TERM_CSI, ...) end
+events.process_response = function(...) events.emit(events.PROCESS_RESPONSE, ...) end
 
 local handlers = {}
 
diff --git a/vis-lua.c b/vis-lua.c
index 9bf5629..1bfeabb 100644
--- a/vis-lua.c
+++ b/vis-lua.c
@@ -23,6 +23,7 @@
 
 #include "vis-lua.h"
 #include "vis-core.h"
+#include "vis-subprocess.h"
 #include "text-motions.h"
 #include "util.h"
 
@@ -52,6 +53,13 @@
 #define debug(...) do { } while (0)
 #endif
 
+typedef struct {
+	/* Lua stream structure for the process input stream */
+	FILE *f;
+	lua_CFunction closef;
+	Process *handler;
+} ProcessStream;
+
 static void window_status_update(Vis *vis, Win *win) {
 	char left_parts[4][255] = { "", "", "", "" };
 	char right_parts[4][32] = { "", "", "", "" };
@@ -162,6 +170,9 @@ void vis_lua_win_close(Vis *vis, Win *win) { }
 void vis_lua_win_highlight(Vis *vis, Win *win) { }
 void vis_lua_win_status(Vis *vis, Win *win) { window_status_update(vis, win); }
 void vis_lua_term_csi(Vis *vis, const long *csi) { }
+void vis_lua_process_response(Vis *vis, const char *name,
+                              char *buffer, size_t len, ResponseType rtype) { }
+
 
 #else
 
@@ -1367,6 +1378,47 @@ static int redraw(lua_State *L) {
 	vis_redraw(vis);
 	return 0;
 }
+/***
+ * Closes a stream returned by @{Vis.communicate}.
+ *
+ * @function close
+ * @tparam io.file inputfd the stream to be closed
+ * @treturn bool the same with @{io.close}
+ */
+static int close_subprocess(lua_State *L) {
+	luaL_Stream *file = luaL_checkudata(L, -1, "FILE*");
+	int result = fclose(file->f);
+	if (result == 0) {
+		file->f = NULL;
+		file->closef = NULL;
+	}
+	return luaL_fileresult(L, result == 0, NULL);
+}
+/***
+ * Open new process and return its input handler.
+ * When the process will quit or will output anything to stdout or stderr,
+ * the @{process_response} event will be fired.
+ *
+ * The editor core won't be blocked while the external process is running.
+ *
+ * @function communicate
+ * @tparam string name the name of subprocess (to distinguish processes in the @{process_response} event)
+ * @tparam string command the command to execute
+ * @return the file handle to write data to the process, in case of error the return values are equivalent to @{io.open} error values.
+ */
+static int communicate_func(lua_State *L) {
+	Vis *vis = obj_ref_check(L, 1, "vis");
+	const char *name = luaL_checkstring(L, 2);
+	const char *cmd = luaL_checkstring(L, 3);
+	ProcessStream *inputfd = (ProcessStream *)lua_newuserdata(L, sizeof(ProcessStream));
+	luaL_setmetatable(L, LUA_FILEHANDLE);
+	inputfd->handler = vis_process_communicate(vis, name, cmd, (void **)(&(inputfd->closef)));
+	if (inputfd->handler) {
+		inputfd->f = fdopen(inputfd->handler->inpfd, "w");
+		inputfd->closef = &close_subprocess;
+	}
+	return inputfd->f ? 1 : luaL_fileresult(L, inputfd->f != NULL, name);
+}
 /***
  * Currently active window.
  * @tfield Window win
@@ -1524,6 +1576,7 @@ static const struct luaL_Reg vis_lua[] = {
 	{ "exit", exit_func },
 	{ "pipe", pipe_func },
 	{ "redraw", redraw },
+	{ "communicate", communicate_func },
 	{ "__index", vis_index },
 	{ "__newindex", vis_newindex },
 	{ NULL, NULL },
@@ -3148,5 +3201,34 @@ void vis_lua_term_csi(Vis *vis, const long *csi) {
 	}
 	lua_pop(L, 1);
 }
+/***
+ * The response received from the process started via @{Vis:communicate}.
+ * @function process_response
+ * @tparam string name the name of process given to @{Vis:communicate}
+ * @tparam string response_type can be "STDOUT" or "STDERR" if new output was received in corresponding channel, "SIGNAL" if the process was terminated by a signal or "EXIT" when the process terminated normally
+ * @tparam string|int buffer the available content sent by process; it becomes the exit code number if response\_type is "EXIT", or the signal number if response\_type is "SIGNAL"
+ */
+void vis_lua_process_response(Vis *vis, const char *name,
+                              char *buffer, size_t len, ResponseType rtype) {
+	lua_State *L = vis->lua;
+	if (!L)
+		return;
+	vis_lua_event_get(L, "process_response");
+	if (lua_isfunction(L, -1)) {
+		lua_pushstring(L, name);
+		if (rtype == EXIT || rtype == SIGNAL)
+			lua_pushinteger(L, len);
+		else
+			lua_pushlstring(L, buffer, len);
+		switch (rtype){
+		case STDOUT: lua_pushstring(L, "STDOUT"); break;
+		case STDERR: lua_pushstring(L, "STDERR"); break;
+		case SIGNAL: lua_pushstring(L, "SIGNAL"); break;
+		case EXIT: lua_pushstring(L, "EXIT"); break;
+		}
+		pcall(vis, L, 3, 0);
+	}
+	lua_pop(L, 1);
+}
 
 #endif
diff --git a/vis-lua.h b/vis-lua.h
index da64233..914f590 100644
--- a/vis-lua.h
+++ b/vis-lua.h
@@ -7,10 +7,11 @@
 #include <lauxlib.h>
 #else
 typedef struct lua_State lua_State;
+typedef void* lua_CFunction;
 #endif
 
 #include "vis.h"
-
+#include "vis-subprocess.h"
 /* add a directory to consider when loading lua files */
 bool vis_lua_path_add(Vis*, const char *path);
 /* get semicolon separated list of paths to load lua files
@@ -38,5 +39,6 @@ void vis_lua_win_close(Vis*, Win*);
 void vis_lua_win_highlight(Vis*, Win*);
 void vis_lua_win_status(Vis*, Win*);
 void vis_lua_term_csi(Vis*, const long *);
+void vis_lua_process_response(Vis *, const char *, char *, size_t, ResponseType);
 
 #endif
diff --git a/vis-subprocess.c b/vis-subprocess.c
new file mode 100644
index 0000000..fa8f7cd
--- /dev/null
+++ b/vis-subprocess.c
@@ -0,0 +1,176 @@
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/wait.h>
+#include "vis-lua.h"
+#include "vis-subprocess.h"
+
+/* Maximum amount of data what can be read from IPC pipe per event */
+#define MAXBUFFER 1024
+
+/* Pool of information about currently running subprocesses */
+static Process *process_pool;
+
+Process *new_in_pool() {
+	/* Adds new empty process information structure to the process pool and
+	 * returns it */
+	Process *newprocess = (Process *)malloc(sizeof(Process));
+	if (!newprocess) return NULL;
+	newprocess->next = process_pool;
+	process_pool = newprocess;
+	return newprocess;
+}
+
+void destroy(Process **pointer) {
+	/* Removes the subprocess information from the pool, sets invalidator to NULL
+	 * and frees resources. */
+	Process *target = *pointer;
+	if (target->outfd != -1) close(target->outfd);
+	if (target->errfd != -1) close(target->errfd);
+	if (target->inpfd != -1) close(target->inpfd);
+	/* marking stream as closed for lua */
+	if (target->invalidator) *(target->invalidator) = NULL;
+	if (target->name) free(target->name);
+	*pointer = target->next;
+	free(target);
+}
+
+Process *vis_process_communicate(Vis *vis, const char *name,
+                                 const char *command, void **invalidator) {
+	/* Starts new subprocess by passing the `command` to the shell and
+	 * returns the subprocess information structure, containing file descriptors
+	 * of the process.
+	 * Also stores the subprocess information to the internal pool to track
+	 * its status and responses.
+	 * `name` - the string than should contain an unique name of the subprocess.
+	 * This name will be passed to the PROCESS_RESPONSE event handler
+	 * to distinguish running subprocesses.
+	 * `invalidator` - a pointer to the pointer which shows that the subprocess
+	 * is invalid when set to NULL. When subprocess dies, it is being set to NULL.
+	 * If the pointer is set to NULL by an external code, the subprocess will be
+	 * killed on the next main loop iteration. */
+	int pin[2], pout[2], perr[2];
+	pid_t pid = (pid_t)-1;
+	if (pipe(perr) == -1) goto closeerr;
+	if (pipe(pout) == -1) goto closeouterr;
+	if (pipe(pin) == -1) goto closeall;
+	pid = fork();
+	if (pid == -1)
+		vis_info_show(vis, "fork failed: %s", strerror(errno));
+	else if (pid == 0){ /* child process */
+		sigset_t sigterm_mask;
+		sigemptyset(&sigterm_mask);
+		sigaddset(&sigterm_mask, SIGTERM);
+		if (sigprocmask(SIG_UNBLOCK, &sigterm_mask, NULL) == -1) {
+			fprintf(stderr, "failed to reset signal mask");
+			exit(EXIT_FAILURE);
+		}
+		dup2(pin[0], STDIN_FILENO);
+		dup2(pout[1], STDOUT_FILENO);
+		dup2(perr[1], STDERR_FILENO);
+	}
+	else { /* main process */
+		Process *new = new_in_pool();
+		if (!new) {
+			vis_info_show(vis, "Can not create process: %s", strerror(errno));
+			goto closeall;
+		}
+		new->name = strdup(name);
+		if (!new->name) {
+			vis_info_show(vis, "Can not copy process name: %s", strerror(errno));
+			/* pop top element (which is `new`) from the pool */
+			destroy(&process_pool);
+			goto closeall;
+		}
+		new->outfd = pout[0];
+		new->errfd = perr[0];
+		new->inpfd = pin[1];
+		new->pid = pid;
+		new->invalidator = invalidator;
+		close(pin[0]);
+		close(pout[1]);
+		close(perr[1]);
+		return new;
+	}
+closeall:
+	close(pin[0]);
+	close(pin[1]);
+closeouterr:
+	close(pout[0]);
+	close(pout[1]);
+closeerr:
+	close(perr[0]);
+	close(perr[1]);
+	if (pid == 0) { /* start command in child process */
+		execlp(vis->shell, vis->shell, "-c", command, (char*)NULL);
+		fprintf(stderr, "exec failed: %s(%d)\n", strerror(errno), errno);
+		exit(1);
+	}
+	else
+		vis_info_show(vis, "process creation failed: %s", strerror(errno));
+	return NULL;
+}
+
+int vis_process_before_tick(fd_set *readfds) {
+	/* Adds file descriptors of currently running subprocesses to the `readfds`
+	 * to track their readiness and returns maximum file descriptor value
+	 * to pass it to the `pselect` call */
+	Process **pointer = &process_pool;
+	int maxfd = 0;
+	while (*pointer) {
+		Process *current = *pointer;
+		if (current->outfd != -1) {
+			FD_SET(current->outfd, readfds);
+			maxfd = maxfd < current->outfd ? current->outfd : maxfd;
+		}
+		if (current->errfd != -1) {
+			FD_SET(current->errfd, readfds);
+			maxfd = maxfd < current->errfd ? current->errfd : maxfd;
+		}
+		pointer = &current->next;
+	}
+	return maxfd;
+}
+
+void read_and_fire(Vis* vis, int fd, const char *name, ResponseType rtype) {
+	/* Reads data from the given subprocess file descriptor `fd` and fires
+	 * the PROCESS_RESPONSE event in Lua with given subprocess `name`,
+	 * `rtype` and the read data as arguments. */
+	static char buffer[MAXBUFFER];
+	size_t obtained = read(fd, &buffer, MAXBUFFER-1);
+	if (obtained > 0)
+		vis_lua_process_response(vis, name, buffer, obtained, rtype);
+}
+
+void vis_process_tick(Vis *vis, fd_set *readfds) {
+	/* Checks if `readfds` contains file discriptors of subprocesses from
+	 * the pool. If so, reads the data from them and fires corresponding events.
+	 * Also checks if subprocesses from pool is dead or need to be killed then
+	 * raises event or kills it if necessary. */
+	Process **pointer = &process_pool;
+	while (*pointer) {
+		Process *current = *pointer;
+		if (current->outfd != -1 && FD_ISSET(current->outfd, readfds))
+			read_and_fire(vis, current->outfd, current->name, STDOUT);
+		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);
+		else
+			vis_lua_process_response(vis, current->name, NULL, WEXITSTATUS(status), EXIT);
+		destroy(pointer);
+	}
+}
diff --git a/vis-subprocess.h b/vis-subprocess.h
new file mode 100644
index 0000000..ae25e21
--- /dev/null
+++ b/vis-subprocess.h
@@ -0,0 +1,23 @@
+#ifndef VIS_SUBPROCESS_H
+#define VIS_SUBPROCESS_H
+#include "vis-core.h"
+#include <sys/select.h>
+
+struct Process {
+	char *name;
+	int outfd;
+	int errfd;
+	int inpfd;
+	pid_t pid;
+	void **invalidator;
+	struct Process *next;
+};
+
+typedef struct Process Process;
+typedef enum { STDOUT, STDERR, SIGNAL, EXIT } ResponseType;
+
+Process *vis_process_communicate(Vis *, const char *command, const char *name,
+                                 void **invalidator);
+int vis_process_before_tick(fd_set *);
+void vis_process_tick(Vis *, fd_set *);
+#endif
diff --git a/vis.c b/vis.c
index f21efa8..24e7f65 100644
--- a/vis.c
+++ b/vis.c
@@ -28,6 +28,7 @@
 #include "vis-core.h"
 #include "sam.h"
 #include "ui.h"
+#include "vis-subprocess.h"
 
 
 static void macro_replay(Vis *vis, const Macro *macro);
@@ -1429,7 +1430,8 @@ int vis_run(Vis *vis) {
 
 		vis_update(vis);
 		idle.tv_sec = vis->mode->idle_timeout;
-		int r = pselect(1, &fds, NULL, NULL, timeout, &emptyset);
+		int r = pselect(vis_process_before_tick(&fds) + 1, &fds, NULL, NULL,
+		                timeout, &emptyset);
 		if (r == -1 && errno == EINTR)
 			continue;
 
@@ -1437,6 +1439,7 @@ int vis_run(Vis *vis) {
 			/* TODO save all pending changes to a ~suffixed file */
 			vis_die(vis, "Error in mainloop: %s\n", strerror(errno));
 		}
+		vis_process_tick(vis, &fds);
 
 		if (!FD_ISSET(STDIN_FILENO, &fds)) {
 			if (vis->mode->idle)

That’s 424 line, so I hoped whether you can get some organization to this.

[mcepl]

I am still not a real C programmer, so just one (probably nonsensical) comment:

• shouldn't that definition of ProcessStream be in the header file?

[xomachine]

That’s 424 line, so I hoped whether you can get some organization to this.

Maybe I missed the point, but according to the links you provided, the patch should implement one feature - no more, no less. So there is no point in splitting the patch to multiple parts that will lead to inability to compile the editor on some intermediate commit or to the dead code. And I did not rewrite the whole editor. My patch mostly adds new code, so reviewing those changes should not take that much effort as it would take in case of replacing huge amounts of existing code.

On other hand I can try to implement partially working feature. E. g. in the first commit only ability to run a process and send data to its stdin, the second one - event handling, the third - ability to stop a process by closing its handle and so on... But I doubt that it will be more readable and is a good practice overall.

* shouldn't that definition of `ProcessStream` be in the header file?

I always try to keep headers as small as possible because their content is included to many compilation units therefore increases build time. So if the definition is not needed outside one compilation unit, I put it into a source file. It is especially important in C++ with its monstrous templates and long compile times, and I thought that it also a good practice in C. If that is a problem - i will move the ProcessStream struct into the header.

[rnpnr]

I agree. I don't see anyway of splitting this up into subcommits.

I also don't think ProcessStream should be in the header. I would argue that since its only used in a single place it shouldn't even be type defined. I think its better to just leave it as a named struct that is local to communicate_func().

Is there a minimal example of some lua code that makes use the exposed features?

[xomachine]

Is there a minimal example of some lua code that makes use the exposed features?

One example can be found in the test code (https://github.com/martanne/vis-test/pull/12/files#diff-8e2c47bd7e90844a45b8b2e5c9c6995fc3431086494baf17617c924dca8abf8dR67) Another is in my implementation of the nimsuggest plugin (which actually is not maintained for a long time) https://github.com/xomachine/nis/blob/async/sessions.lua#L85

[tosch42]

There are some smaller things that should be changed too. For example, it's unnecessary to cast the result of a malloc(3) call. Empty parameter lists should be func(void) instead of func(). All occurrences of MAXBUFFER should be replaced by PIPE_BUF. typedef the Process structure before its declaration, that way you can just use Process inside of the struct. All the local functions should be declared static. The naming scheme of those functions is a bit inconsistent to me. I'd rather call new_in_pool() new_process() and instead of destroy() I'd say destroy_process(). You get the idea. Then, if you have to write a comment that explains a function, please place it above the function, not into it. If it was up to me, I'd toss most of the comments. They don't seem necessary to me. That's just the little things that come to mind, unfortunately I currently don't have the time to review this in any more depth.

[mcepl]

Maybe I missed the point, but according to the links you provided, the patch should implement one feature - no more, no less.

Just for the record that’s not exactly what those links meant (AFAIK): the main point is that instead of the chronological commits (“Let’s try this …”, “Whoops, that didn’t work …”, “It’s Friday, saving my week work before the weekend” etc.) the branch for review should be organized topically (“necessary changes in C making the ground for everything else”, “implementation of Lua interface”, “documentation”, “tests” or something like it). How big or small those topics are and how many commits should be in the branch is completely orthogonal to this organization.

And yes, 400+ lines patch is probably somewhere on the edge of being small enough not to split it at all.

[xomachine]

There are some smaller things that should be changed too.

I tried to fix the problems you mentioned. At least some of them.

Each commit addresses one issue found in code. Or should it be one commit per review iteration? Or I should fixup my previous commit to make the history cleaner? Now I think I completely lost the purpose of the commit history. =')

[rnpnr]

Maybe I'm doing something wrong but overall it seems this doesn't behave as intended. My example test:

vis.events.subscribe(vis.events.INIT, function()
	vis:map(vis.modes.NORMAL, " t", function()
		vis:communicate("timer", "sleep 10; echo timer done")
	end, "start a 10s timer")
end)
vis.events.subscribe(vis.events.PROCESS_RESPONSE, function(name, resp, buf)
	vis:message("PROCESS_RESPONSE\n\tname = " .. name)
	vis:message("\tresp_type = " .. resp)
	vis:message("\tbuffer = " .. buf)
end)

After I press ␣t I can move the cursor for a little bit, then I can't move the cursor for some seconds, and finally I get this printed to the message window:

PROCESS_RESPONSE
	name = timer
	resp_type = 143
	buffer = EXIT

[fischerling]

Maybe I'm doing something wrong but overall it seems this doesn't behave as intended. My example test:

vis.events.subscribe(vis.events.INIT, function()
	vis:map(vis.modes.NORMAL, " t", function()
		vis:communicate("timer", "sleep 10; echo timer done")
	end, "start a 10s timer")
end)
vis.events.subscribe(vis.events.PROCESS_RESPONSE, function(name, resp, buf)
	vis:message("PROCESS_RESPONSE\n\tname = " .. name)
	vis:message("\tresp_type = " .. resp)
	vis:message("\tbuffer = " .. buf)
end)

After I press ␣t I can move the cursor for a little bit, then I can't move the cursor for some seconds, and finally I get this printed to the message window:

PROCESS_RESPONSE
	name = timer
	resp_type = 143
	buffer = EXIT

For me your test case works with my own visrc.lua file and bunch of plugins and configured events without problems. I can continue using vis normally during the ten seconds and afterwards I get the expected output:

PROCESS_RESPONSE
    name = timer
    resp_type = timer done

    buffer = STDOUT
PROCESS_RESPONSE
    name = timer
    resp_type = 0
    buffer = EXIT

My login shell is fish.

Sometimes when trying this simple test case with a minimal vis config the command was terminated by SIGTERM, but I can not reliably reproduce it.

[xomachine]

Thank you for review! Though I can not guarantee that I will answer or fix everything soon, I will definitely look into it.

Sometimes when trying this simple test case with a minimal vis config the command was terminated by SIGTERM, but I can not reliably reproduce it.

This happens because the lua garbage collector collects the process handle and therefore vis kills spawned process. To prevent it you should put the result of the vis:communicate() to a variable and keep it in scope until the process is finished.

[mcepl]

Maybe I'm doing something wrong but overall it seems this doesn't behave as intended. My example test:

vis.events.subscribe(vis.events.INIT, function()
	vis:map(vis.modes.NORMAL, " t", function()
		vis:communicate("timer", "sleep 10; echo timer done")
	end, "start a 10s timer")
end)
vis.events.subscribe(vis.events.PROCESS_RESPONSE, function(name, resp, buf)
	vis:message("PROCESS_RESPONSE\n\tname = " .. name)
	vis:message("\tresp_type = " .. resp)
	vis:message("\tbuffer = " .. buf)
end)

After I press ␣t I can move the cursor for a little bit, then I can't move the cursor for some seconds, and finally I get this printed to the message window:

PROCESS_RESPONSE
	name = timer
	resp_type = 143
	buffer = EXIT

Works for me just fine (as applied in https://git.sr.ht/~mcepl/vis), I had to just switch off vis-lspc, because it got confused.

[rnpnr]

    resp_type = timer done

    buffer = STDOUT

But even this isn't correct. resp_type should be STDOUT and buffer should be timer done\n.

This happens because the lua garbage collector collects the process handle and therefore vis kills spawned process

This seems to be the issue with my example. It works fine if I have a global variable holding the handle. A note about this should definitely be added to the documentation. Interestingly in only happens when I actually try to do something. If I just start the timer and do nothing the process finishes successfully. Maybe that is expected though.

Is there a way we can avoid this altogether? It seems like an annoying limitation.

[xomachine]

But even this isn't correct. resp_type should be STDOUT and buffer should be timer done\n.

The order of input parameters is wrong in the example. According to the documentation the second parameters is the answer itself, and the third one is the type. Maybe it is counterintuitive and should be changed.

I got caught by my own trap. Yes, that is a bug.

* @function process_response
* @tparam string name the name of process given to @{Vis:communicate}
* @tparam string response_type can be "STDOUT" or "STDERR" if new output was received in corresponding channel, "SIGNAL" if the process was terminated by a signal or "EXIT" when the process terminated normally
* @tparam string|int buffer the available content sent by process; it becomes the exit code number if response\_type is "EXIT", or the signal number if response\_type is "SIGNAL"

Is there a way we can avoid this altogether? It seems like an annoying limitation.

It is done this way intentionally. If we don't keep track of processes spawned by vis it will lead to possible process leak. E. g. some process spawned by vis hangs for some reason and does not terminate in a proper way - in that case lua code is unable to kill it and the process will be in background until the editor is terminated.

[rnpnr]

I got caught by my own trap

No worries, I probably should have called the variable rtype not resp.

If we don't keep track of processes spawned by vis it will lead to possible process leak

That makes sense, I wasn't really thinking about all potential uses. In any case we need to make sure that it is documented otherwise we will get lots of bug reports from doing exactly what I did.

[pippi1otta]

If we don't keep track of processes spawned by vis it will lead to possible process leak. [...] the process will be in background until the editor is terminated. ~@xomachine

I'm wondering if the benefits of the defensive programming employed here outweigh the inconvenience of having to keep the return value in scope.

Do people keep vis open for long enough for such a leak to be an issue?

Due to vis' simplistic handling of multiple windows and files, and rudimentary command line, I tend to close and re-open vis quite a lot, instead of "living" in it, like I would do with, say, Emacs (or AutoCAD).

And if closing the editor is all it takes to reap the processes, it looks like a small price to pay (if you even notice such a leak).

Some commands that one might want to execute in the background, just don't take input. In these cases, having to keep an input fd in scope only to keep your process from being killed will feel like an... overkill.

Is there a way we can avoid this altogether? ~@rnpnr

Maybe an extra, optional, boolean argument to vis:communicate(), signalling if you want to send input or not. If not, the return value will be nil instead of a fd, and the garbage collector will not kill your process.

I have no idea whether the above is possible. Just thinking out loud here.

[mcepl]

Do people keep vis open for long enough for such a leak to be an issue?

It depends on what you want to do with it. One of my ideas about vis:communicate() would be to use it in vis-spellchecker and running hunspell on the background. Writing a prose can make you sit in one vis session for a long time.

[rnpnr]

running hunspell on the background

But in that case you would want to keep the file handle around to keep writing to it as new text is input. @pippi1otta is talking about examples like my test where you have something that needs to be run in the background but doesn't need data written to it and can finish whenever it finishes. I won't block merging this over such an addition but I think it could be an acceptable extension later on.

[mcepl]

But in that case you would want to keep the file handle around to keep writing to it as new text is input. @pippi1otta is talking about examples like my test where you have something that needs to be run in the background but doesn't need data written to it and can finish whenever it finishes. I won't block merging this over such an addition but I think it could be an acceptable extension later on.

That’s correct.