gvfs-helper-client: clean up server process(es)

On Linux, the following command would cause the terminal to be stuck
waiting:

  git fetch origin foobar

The issue would be that the fetch would fail with the error

  fatal: couldn't find remote ref foobar

but the underlying git-gvfs-helper process wouldn't die. The
`subprocess_exit_handler()` method would close its stdin and stdout, but
that wouldn't be enough to cause the process to end, even though the
`packet_read_line_gently()` call that is run in `do_sub_cmd__server()`
in a loop should return -1 and the process should the terminate
peacefully.

While it is unclear why this does not happen, there may be other
conditions where the `gvfs-helper` process would not terminate. Let's
ensure that the gvfs-helper-client process cleans up the gvfs-helper
server processes that it spawned upon exit.

Reported-by: Stuart Wilcox Humilde <stuartwilcox@microsoft.com>
Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Author: derrickstolee

gvfs-helper-client.c

@@ -335,6 +335,62 @@ static void gh_client__choose_odb(void)
 	}
 }
 
+/*
+ * Custom exit handler for the `gvfs-helper` subprocesses.
+ *
+ * These helper subprocesses will keep waiting for input until they are
+ * stopped. The default `subprocess_exit_handler()` will instead wait for
+ * the subprocess to exit, which is not what we want: In case of a fatal
+ * error, the Git process will exit and the `gvfs-helper` subprocesses will
+ * need to be stopped explicitly.
+ *
+ * Sadly, there is no way to pass the `struct subprocess_entry` instance to
+ * the `clean_on_exit_handler`, it will only receive the `struct child_process`
+ * pointer. But since we know that that pointer points to a field of
+ * the `subprocess_entry` struct, we can use `container_of()` to get that
+ * instance and then use it to stop the subprocess as desired.
+ *
+ * However, that would only take care of _stopping_ the process, not of
+ * releasing the memory allocated for the hashmap entry (let alone for the
+ * hashmap itself). Therefore, we just use the first exit handler call (if any)
+ * to simply stop all of the `gvfs-helper` processes.
+ */
+static void gh_client__subprocess_exit_handler(struct child_process *cp UNUSED)
+{
+	struct gh_server__process *entry, **entries;
+	struct hashmap_iter iter;
+	size_t i = 0, size;
+
+	if (!gh_server__subprocess_map_initialized)
+		return;
+
+	/*
+	 * Since `subprocess_stop()` modifies the hashmap by removing the
+	 * respective subprocess, we cannot use `hashmap_for_each_entry()`
+	 * directly, but have to copy the entries to an array first.
+	 */
+	size = hashmap_get_size(&gh_server__subprocess_map);
+	ALLOC_ARRAY(entries, size);
+	hashmap_for_each_entry(&gh_server__subprocess_map,
+			       &iter, entry, subprocess)
+		entries[i++] = entry;
+	if (i != size)
+		BUG("gh_server__subprocess_map: size mismatch: "
+		    "%"PRIuMAX" != %"PRIuMAX,
+		      (uintmax_t)i, (uintmax_t)size);
+	while(i--) {
+		subprocess_stop(&gh_server__subprocess_map,
+				&entries[i]->subprocess);
+		FREE_AND_NULL(entries[i]);
+	}
+
+	FREE_AND_NULL(entries);
+
+	hashmap_clear_and_free(&gh_server__subprocess_map,
+			       struct gh_server__process, subprocess);
+	gh_server__subprocess_map_initialized = 0;
+}
+
 static struct gh_server__process *gh_client__find_long_running_process(
 	unsigned int cap_needed)
 {
@@ -386,6 +442,9 @@ static struct gh_server__process *gh_client__find_long_running_process(
 					  &entry->subprocess, 1,
 					  &argv, gh_client__start_fn))
 			FREE_AND_NULL(entry);
+		else
+			entry->subprocess.process.clean_on_exit_handler =
+				gh_client__subprocess_exit_handler;
 	}
 
 	if (entry &&

t/t9210-scalar.sh

@@ -426,6 +426,10 @@ test_expect_success '`scalar clone` with GVFS-enabled server' '
 	)
 '
 
+test_expect_success 'fetch <non-existent> does not hang in gvfs-helper' '
+	test_must_fail git -C using-gvfs/src fetch origin does-not-exist
+'
+
 test_expect_success '`scalar clone --no-gvfs-protocol` skips gvfs/config' '
 	# the fake cache server requires fake authentication &&
 	git config --global core.askPass true &&