gvfs-helper-client: clean up server process #756
Conversation
|
Promoting to a full PR after trying to explore the option of being better bout |
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 <[email protected]> Signed-off-by: Derrick Stolee <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
gvfs-helper-client.c
Outdated
| @@ -48,6 +58,8 @@ static int gh_client__start_fn(struct subprocess_entry *subprocess) | |||
|
|
|||
| struct gh_server__process *entry = (struct gh_server__process *)subprocess; | |||
|
|
|||
| server_process_pid = subprocess->process.pid; | |||
| atexit(cleanup_server_process_on_exit); | |||
There was a problem hiding this comment.
While it may be interesting to find out why the process does not finish when stdin closes (looks like the packet_read_line_gently() call that should return -1 does not? Or maybe the problem is that child processes continue to exist as zombie processes on Linux until a waitpid() of their parent process releases them?), an atexit() handler definitely takes care of cleaning up the server process.
However, there may be more than one gvfs-helper process to clean up. @derrickstolee could you test whether b8d292f (pushed to microsoft/git as gvfs-helper-linux-term) addresses the problem you observed, too?
I had to change where the atexit() was registered in order to make sure things ran at the right place.
derrickstolee
force-pushed
the
gvfs-helper-linux-term
branch
from
5d57c6d to
68f5d49
Compare
| @@ -48,6 +50,8 @@ static int gh_client__start_fn(struct subprocess_entry *subprocess) | |||
|
|
|||
| struct gh_server__process *entry = (struct gh_server__process *)subprocess; | |||
|
|
|||
| atexit(stop_gh_server_subprocesses); | |||
There was a problem hiding this comment.
Maybe we should move this all the way into the cmd_main() function, to be 100% sure that it is registered, and registered one time only?
There was a problem hiding this comment.
That could work, or I could add a static int registered = 0; into this method. This is definitely on the critical path for starting a process.
There was a problem hiding this comment.
What I do not understand is: the previous location was in the code path that initializes the hashmap, and the new location is in the gh_client__start_fn() function which is passed as a callback a couple lines further down from the previous location. So how is it possible that it was not called early enough in the previous location but it is early enough in the new location?
There was a problem hiding this comment.
I was also confused by this, since I specifically traced that the hashing location was being called but somehow wasn't sticking. I'll look into it some more later this week.
On Linux, the following command would cause the terminal to be stuck waiting:
The issue would be that the fetch would fail with the error
but the underlying
git-gvfs-helperprocess wouldn't die. Thesubprocess_exit_handler()method would close its stdin and stdout, but that wouldn't be enough to cause the process to end.The current approach creates our own
atexit()handler by sending aSIGINTsignal to the child process. This has worked in my end-to-end test.Perhaps another approach would be to change the way git-gvfs-helper server gets messages over
stdinto better terminate when the pipe closes. However, I debugged to the point that I could see thegit-gvfs-helperprocess waiting on a basicread()from the standard library, so it's not mishandling anEOFsignal or anything.