Defunct Git Processes left lying around #1392

@trebonian commented on GitHub (Dec 20, 2017):

I fixed it locally in /vendor/code.gitea.io/git/commit-info.go:
I added a line "defer cmd.Wait() after the cmd.exec of git log

@trebonian commented on GitHub (Dec 20, 2017): I fixed it locally in /vendor/code.gitea.io/git/commit-info.go: I added a line "defer cmd.Wait() after the cmd.exec of git log

GiteaMirror commented

@lunny commented on GitHub (Dec 20, 2017):

@trebonian could you send a PR to github.com/go-gitea/git?

@lunny commented on GitHub (Dec 20, 2017): @trebonian could you send a PR to `github.com/go-gitea/git`?

GiteaMirror commented

@trebonian commented on GitHub (Dec 20, 2017):

Not sure how to do it without a fork of gitea on github.
Here is a "git diff" of my change:

diff --git a/vendor/code.gitea.io/git/commit_info.go b/vendor/code.gitea.io/git/commit_info.go
index 106ebc61..c2691fc6 100644
--- a/vendor/code.gitea.io/git/commit_info.go
+++ b/vendor/code.gitea.io/git/commit_info.go
@@ -198,6 +198,7 @@ func getCommitsInfo(state *getCommitsInfoState) error {
        }
        cmd := exec.CommandContext(ctx, "git", args...)
        cmd.Dir = state.headCommit.repo.Path
+       defer cmd.Wait()

        readCloser, err := cmd.StdoutPipe()
        if err != nil {

EDIT: change the formatting so it's readable // BKC

@trebonian commented on GitHub (Dec 20, 2017): Not sure how to do it without a fork of gitea on github. Here is a "git diff" of my change: ``` diff --git a/vendor/code.gitea.io/git/commit_info.go b/vendor/code.gitea.io/git/commit_info.go index 106ebc61..c2691fc6 100644 --- a/vendor/code.gitea.io/git/commit_info.go +++ b/vendor/code.gitea.io/git/commit_info.go @@ -198,6 +198,7 @@ func getCommitsInfo(state *getCommitsInfoState) error { } cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = state.headCommit.repo.Path + defer cmd.Wait() readCloser, err := cmd.StdoutPipe() if err != nil { ``` EDIT: change the formatting so it's readable // BKC

GiteaMirror commented

@ethantkoenig commented on GitHub (Dec 20, 2017):

I don't think calling cmd.Wait() is the right solution here, since that will block until the git log command completely finishes (which will take a long time for large repos)

@ethantkoenig commented on GitHub (Dec 20, 2017): I don't think calling `cmd.Wait()` is the right solution here, since that will block until the `git log` command completely finishes (which will take a long time for large repos)

GiteaMirror commented

@trebonian commented on GitHub (Dec 20, 2017):

its deferred until this routine returns - which I assumed meant all the log output was processed.
The cmd.Wait() needs to be called before the routine returns. Is there a better place for it?

@trebonian commented on GitHub (Dec 20, 2017): its deferred until this routine returns - which I assumed meant all the log output was processed. The cmd.Wait() needs to be called before the routine returns. Is there a better place for it?

GiteaMirror commented

@ethantkoenig commented on GitHub (Dec 20, 2017):

@trebonian No, the function may return without processing all of the git log output.

@ethantkoenig commented on GitHub (Dec 20, 2017): @trebonian No, the function may return without processing all of the `git log` output.

GiteaMirror commented

@trebonian commented on GitHub (Dec 20, 2017):

@ethantkoenig then whoever processes the last output needs to do a "Wait" otherwise zombies will be left lying around

@trebonian commented on GitHub (Dec 20, 2017): @ethantkoenig then whoever processes the last output needs to do a "Wait" otherwise zombies will be left lying around

GiteaMirror commented

@ethantkoenig commented on GitHub (Dec 20, 2017):

It was my understanding that defer cancel() (here) would clean up the process, but I'll investigate more when I get the chance.

@ethantkoenig commented on GitHub (Dec 20, 2017): It was my understanding that `defer cancel()` ([here](https://github.com/go-gitea/git/blob/4573c63ec9c8257caf11e361c238b6ceb53781d7/commit_info.go#L193)) would clean up the process, but I'll investigate more when I get the chance.

GiteaMirror commented

@trebonian commented on GitHub (Dec 20, 2017):

Thanks - the symptom that I am seeing is that there is a new "git" zombie processes after every invocation of gitCommitsInfo.

I am not sure how the Golang runtime handles the "cancel" but the main process needs to do an OS wait on the child process id (at the linux level). I don't know how it can do this if the only handle to that child process is lost after gitCommitsInfo returns.

@trebonian commented on GitHub (Dec 20, 2017): Thanks - the symptom that I am seeing is that there is a new "git" zombie processes after every invocation of gitCommitsInfo. I am not sure how the Golang runtime handles the "cancel" but the main process needs to do an OS wait on the child process id (at the linux level). I don't know how it can do this if the only handle to that child process is lost after gitCommitsInfo returns.

GiteaMirror commented

@trebonian commented on GitHub (Dec 20, 2017):

PS - it should only call the Wait if we know the child is done - otherwise we will block the main process.

@trebonian commented on GitHub (Dec 20, 2017): PS - it should only call the Wait if we know the child is done - otherwise we will block the main process.

GiteaMirror commented

@lunny commented on GitHub (Dec 20, 2017):

I think @trebonian is right. cmd.Wait is needed if cmd.Start but not cmd.Run.

@lunny commented on GitHub (Dec 20, 2017): I think @trebonian is right. `cmd.Wait` is needed if `cmd.Start` but not `cmd.Run`.

GiteaMirror commented

@trebonian commented on GitHub (Dec 20, 2017):

I think @ethantkoenig is correct, and we don't want the main process to block - but I also think we need to call cmd.Wait() in the main process when the cmd'ed process terminates. Note that the current code does not seem to check for any kind of process error while the cmd is running.

I did the following after the cmd.Start() and it seems to work:
go func () { cmd.Wait() }()

This starts a goroutine that will wait until the process exits. This seems to fix up the zombies - but it's not the nicest.

I am not sure, but I think there needs to be something with a "done" channel to detect and report process errors (like getting killed, or signalled while running), and the cmd process done detection is merged with the other thread done check's lower down.

@trebonian commented on GitHub (Dec 20, 2017): I think @ethantkoenig is correct, and we don't want the main process to block - but I also think we need to call cmd.Wait() in the main process when the cmd'ed process terminates. Note that the current code does not seem to check for any kind of process error while the cmd is running. I did the following after the cmd.Start() and it seems to work: go func () { cmd.Wait() }() This starts a goroutine that will wait until the process exits. This seems to fix up the zombies - but it's not the nicest. I am not sure, but I think there needs to be something with a "done" channel to detect and report process errors (like getting killed, or signalled while running), and the cmd process done detection is merged with the other thread done check's lower down.

GiteaMirror commented

@lunny commented on GitHub (Dec 20, 2017):

go func () { cmd.Wait() }()

seems work. Or maybe handle the error and write to log.

@lunny commented on GitHub (Dec 20, 2017): ```Go go func () { cmd.Wait() }() ``` seems work. Or maybe handle the error and write to log.

GiteaMirror commented