From c01cccfee61148042ba5dbb37e4bb2de181b402b Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 15:53:54 +0100 Subject: [PATCH 1/8] Experiment in using context --- internal/helper/command.go | 6 +++++- internal/service/diff/commit.go | 2 +- internal/service/ref/refname.go | 6 +++--- internal/service/ref/refs.go | 26 ++++++++++++------------ internal/service/ref/refs_test.go | 28 +++++++++++++------------- internal/service/smarthttp/inforefs.go | 9 +++++---- 6 files changed, 41 insertions(+), 36 deletions(-) diff --git a/internal/helper/command.go b/internal/helper/command.go index 46cf1543e8d..80745a240f0 100644 --- a/internal/helper/command.go +++ b/internal/helper/command.go @@ -6,6 +6,8 @@ import ( "os" "os/exec" "syscall" + + "golang.org/x/net/context" ) // Command encapsulates operations with commands creates with NewCommand @@ -21,7 +23,9 @@ func (c *Command) Kill() { } // GitCommandReader creates a git Command with the given args -func GitCommandReader(args ...string) (*Command, error) { +func GitCommandReader(ctx context.Context, args ...string) (*Command, error) { + // TODO: when we switch to Go 1.7, switch to using + // exec.CommandContext return NewCommand(exec.Command("git", args...), nil, nil) } diff --git a/internal/service/diff/commit.go b/internal/service/diff/commit.go index 7ad900f996f..e762989dffa 100644 --- a/internal/service/diff/commit.go +++ b/internal/service/diff/commit.go @@ -25,7 +25,7 @@ func (s *server) CommitDiff(in *pb.CommitDiffRequest, stream pb.Diff_CommitDiffS log.Printf("CommitDiff: RepoPath=%q LeftCommitId=%q RightCommitId=%q", repoPath, leftSha, rightSha) - cmd, err := helper.GitCommandReader("--git-dir", repoPath, "diff", "--full-index", "--find-renames", leftSha, rightSha) + cmd, err := helper.GitCommandReader(stream.Context(), "--git-dir", repoPath, "diff", "--full-index", "--find-renames", leftSha, rightSha) if err != nil { return grpc.Errorf(codes.Internal, "CommitDiff: cmd: %v", err) } diff --git a/internal/service/ref/refname.go b/internal/service/ref/refname.go index d3b402b9414..88631899cea 100644 --- a/internal/service/ref/refname.go +++ b/internal/service/ref/refname.go @@ -28,7 +28,7 @@ func (s *server) FindRefName(ctx context.Context, in *pb.FindRefNameRequest) (*p return nil, grpc.Errorf(codes.InvalidArgument, message) } - ref, err := findRefName(repoPath, in.CommitId, string(in.Prefix)) + ref, err := findRefName(ctx, repoPath, in.CommitId, string(in.Prefix)) if err != nil { return nil, grpc.Errorf(codes.Internal, err.Error()) } @@ -37,8 +37,8 @@ func (s *server) FindRefName(ctx context.Context, in *pb.FindRefNameRequest) (*p } // We assume `path` and `commitID` and `prefix` are non-empty -func findRefName(path, commitID, prefix string) (string, error) { - cmd, err := helper.GitCommandReader("--git-dir", path, "for-each-ref", "--format=%(refname)", "--count=1", prefix, "--contains", commitID) +func findRefName(ctx context.Context, path, commitID, prefix string) (string, error) { + cmd, err := helper.GitCommandReader(ctx, "--git-dir", path, "for-each-ref", "--format=%(refname)", "--count=1", prefix, "--contains", commitID) if err != nil { return "", err } diff --git a/internal/service/ref/refs.go b/internal/service/ref/refs.go index 051e9fb5ac2..c831f82d649 100644 --- a/internal/service/ref/refs.go +++ b/internal/service/ref/refs.go @@ -33,7 +33,7 @@ func handleGitCommand(w refsWriter, r io.Reader) error { return w.Flush() } -func findRefs(writer refsWriter, repo *pb.Repository, pattern string, args ...string) error { +func findRefs(ctx context.Context, writer refsWriter, repo *pb.Repository, pattern string, args ...string) error { repoPath, err := helper.GetRepoPath(repo) if err != nil { return err @@ -49,7 +49,7 @@ func findRefs(writer refsWriter, repo *pb.Repository, pattern string, args ...st args = append(baseArgs, args...) } - cmd, err := helper.GitCommandReader(args...) + cmd, err := helper.GitCommandReader(ctx, args...) if err != nil { return err } @@ -64,18 +64,18 @@ func findRefs(writer refsWriter, repo *pb.Repository, pattern string, args ...st // FindAllBranchNames creates a stream of ref names for all branches in the given repository func (s *server) FindAllBranchNames(in *pb.FindAllBranchNamesRequest, stream pb.Ref_FindAllBranchNamesServer) error { - return findRefs(newFindAllBranchNamesWriter(stream, s.MaxMsgSize), in.Repository, "refs/heads") + return findRefs(stream.Context(), newFindAllBranchNamesWriter(stream, s.MaxMsgSize), in.Repository, "refs/heads") } // FindAllTagNames creates a stream of ref names for all tags in the given repository func (s *server) FindAllTagNames(in *pb.FindAllTagNamesRequest, stream pb.Ref_FindAllTagNamesServer) error { - return findRefs(newFindAllTagNamesWriter(stream, s.MaxMsgSize), in.Repository, "refs/tags") + return findRefs(stream.Context(), newFindAllTagNamesWriter(stream, s.MaxMsgSize), in.Repository, "refs/tags") } -func _findBranchNames(repoPath string) ([][]byte, error) { +func _findBranchNames(ctx context.Context, repoPath string) ([][]byte, error) { var names [][]byte - cmd, err := helper.GitCommandReader("--git-dir", repoPath, "for-each-ref", "refs/heads", "--format=%(refname)") + cmd, err := helper.GitCommandReader(ctx, "--git-dir", repoPath, "for-each-ref", "refs/heads", "--format=%(refname)") if err != nil { return nil, err } @@ -96,10 +96,10 @@ func _findBranchNames(repoPath string) ([][]byte, error) { return names, nil } -func _headReference(repoPath string) ([]byte, error) { +func _headReference(ctx context.Context, repoPath string) ([]byte, error) { var headRef []byte - cmd, err := helper.GitCommandReader("--git-dir", repoPath, "rev-parse", "--symbolic-full-name", "HEAD") + cmd, err := helper.GitCommandReader(ctx, "--git-dir", repoPath, "rev-parse", "--symbolic-full-name", "HEAD") if err != nil { return nil, err } @@ -119,8 +119,8 @@ func _headReference(repoPath string) ([]byte, error) { return headRef, nil } -func defaultBranchName(repoPath string) ([]byte, error) { - branches, err := findBranchNames(repoPath) +func defaultBranchName(ctx context.Context, repoPath string) ([]byte, error) { + branches, err := findBranchNames(ctx, repoPath) if err != nil { return nil, err @@ -137,7 +137,7 @@ func defaultBranchName(repoPath string) ([]byte, error) { } hasMaster := false - headRef, err := headReference(repoPath) + headRef, err := headReference(ctx, repoPath) if err != nil { return nil, err } @@ -167,7 +167,7 @@ func (s *server) FindDefaultBranchName(ctx context.Context, in *pb.FindDefaultBr log.Printf("FindDefaultBranchName: RepoPath=%q", repoPath) - defaultBranchName, err := defaultBranchName(repoPath) + defaultBranchName, err := defaultBranchName(ctx, repoPath) if err != nil { return nil, err } @@ -195,5 +195,5 @@ func (s *server) FindLocalBranches(in *pb.FindLocalBranchesRequest, stream pb.Re sortFlag := "--sort=" + parseSortKey(in.GetSortBy()) writer := newFindLocalBranchesWriter(stream, s.MaxMsgSize) - return findRefs(writer, in.Repository, "refs/heads", formatFlag, sortFlag) + return findRefs(stream.Context(), writer, in.Repository, "refs/heads", formatFlag, sortFlag) } diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go index ae14e48ebd9..1c4d57513e3 100644 --- a/internal/service/ref/refs_test.go +++ b/internal/service/ref/refs_test.go @@ -175,7 +175,7 @@ func TestInvalidRepoFindAllTagNamesRequest(t *testing.T) { } func TestHeadReference(t *testing.T) { - headRef, err := headReference(testRepoPath) + headRef, err := headReference(context.Background(), testRepoPath) if err != nil { t.Fatal(err) } @@ -193,47 +193,47 @@ func TestDefaultBranchName(t *testing.T) { testCases := []struct { desc string - findBranchNames func(string) ([][]byte, error) - headReference func(string) ([]byte, error) + findBranchNames func(context.Context, string) ([][]byte, error) + headReference func(context.Context, string) ([]byte, error) expected []byte }{ { desc: "Get first branch when only one branch exists", expected: []byte("refs/heads/foo"), - findBranchNames: func(string) ([][]byte, error) { + findBranchNames: func(context.Context, string) ([][]byte, error) { return [][]byte{[]byte("refs/heads/foo")}, nil }, - headReference: func(string) ([]byte, error) { return nil, nil }, + headReference: func(context.Context, string) ([]byte, error) { return nil, nil }, }, { desc: "Get empy ref if no branches exists", expected: nil, - findBranchNames: func(string) ([][]byte, error) { return [][]byte{}, nil }, - headReference: func(string) ([]byte, error) { return nil, nil }, + findBranchNames: func(context.Context, string) ([][]byte, error) { return [][]byte{}, nil }, + headReference: func(context.Context, string) ([]byte, error) { return nil, nil }, }, { desc: "Get the name of the head reference when more than one branch exists", expected: []byte("refs/heads/bar"), - findBranchNames: func(string) ([][]byte, error) { + findBranchNames: func(context.Context, string) ([][]byte, error) { return [][]byte{[]byte("refs/heads/foo"), []byte("refs/heads/bar")}, nil }, - headReference: func(string) ([]byte, error) { return []byte("refs/heads/bar"), nil }, + headReference: func(context.Context, string) ([]byte, error) { return []byte("refs/heads/bar"), nil }, }, { desc: "Get `ref/heads/master` when several branches exist", expected: []byte("refs/heads/master"), - findBranchNames: func(string) ([][]byte, error) { + findBranchNames: func(context.Context, string) ([][]byte, error) { return [][]byte{[]byte("refs/heads/foo"), []byte("refs/heads/master"), []byte("refs/heads/bar")}, nil }, - headReference: func(string) ([]byte, error) { return nil, nil }, + headReference: func(context.Context, string) ([]byte, error) { return nil, nil }, }, { desc: "Get the name of the first branch when several branches exists and no other conditions are met", expected: []byte("refs/heads/foo"), - findBranchNames: func(string) ([][]byte, error) { + findBranchNames: func(context.Context, string) ([][]byte, error) { return [][]byte{[]byte("refs/heads/foo"), []byte("refs/heads/bar"), []byte("refs/heads/baz")}, nil }, - headReference: func(string) ([]byte, error) { return nil, nil }, + headReference: func(context.Context, string) ([]byte, error) { return nil, nil }, }, } @@ -241,7 +241,7 @@ func TestDefaultBranchName(t *testing.T) { findBranchNames = testCase.findBranchNames headReference = testCase.headReference - defaultBranch, err := defaultBranchName("") + defaultBranch, err := defaultBranchName(context.Background(), "") if err != nil { t.Fatal(err) } diff --git a/internal/service/smarthttp/inforefs.go b/internal/service/smarthttp/inforefs.go index 0ea48d52967..095b83975a5 100644 --- a/internal/service/smarthttp/inforefs.go +++ b/internal/service/smarthttp/inforefs.go @@ -8,6 +8,7 @@ import ( pb "gitlab.com/gitlab-org/gitaly-proto/go" "gitlab.com/gitlab-org/gitaly/internal/helper" + "golang.org/x/net/context" "google.golang.org/grpc" "google.golang.org/grpc/codes" ) @@ -29,20 +30,20 @@ func (w infoRefsResponseWriter) Write(p []byte) (int, error) { } func (s *server) InfoRefsUploadPack(in *pb.InfoRefsRequest, stream pb.SmartHTTP_InfoRefsUploadPackServer) error { - return handleInfoRefs("upload-pack", in.Repository, infoRefsResponseWriter{stream}) + return handleInfoRefs(stream.Context(), "upload-pack", in.Repository, infoRefsResponseWriter{stream}) } func (s *server) InfoRefsReceivePack(in *pb.InfoRefsRequest, stream pb.SmartHTTP_InfoRefsReceivePackServer) error { - return handleInfoRefs("receive-pack", in.Repository, infoRefsResponseWriter{stream}) + return handleInfoRefs(stream.Context(), "receive-pack", in.Repository, infoRefsResponseWriter{stream}) } -func handleInfoRefs(service string, repo *pb.Repository, w io.Writer) error { +func handleInfoRefs(ctx context.Context, service string, repo *pb.Repository, w io.Writer) error { repoPath, err := helper.GetRepoPath(repo) if err != nil { return err } - cmd, err := helper.GitCommandReader(service, "--stateless-rpc", "--advertise-refs", repoPath) + cmd, err := helper.GitCommandReader(ctx, service, "--stateless-rpc", "--advertise-refs", repoPath) if err != nil { return grpc.Errorf(codes.Internal, "GetInfoRefs: cmd: %v", err) } -- GitLab From 1e7a5c6dd3d54974b000933da9dc966e2e213cea Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 16:25:21 +0100 Subject: [PATCH 2/8] Handle go1.7 CommandContext and also go1.6 --- internal/helper/command.go | 2 +- internal/helper/command_wrapper_go1.6.go | 14 ++++++++++++++ internal/helper/command_wrapper_go17.go | 14 ++++++++++++++ internal/service/commit/isancestor.go | 7 +++---- internal/service/smarthttp/receive_pack.go | 3 +-- internal/service/smarthttp/upload_pack.go | 3 +-- 6 files changed, 34 insertions(+), 9 deletions(-) create mode 100644 internal/helper/command_wrapper_go1.6.go create mode 100644 internal/helper/command_wrapper_go17.go diff --git a/internal/helper/command.go b/internal/helper/command.go index 80745a240f0..5b9288757ba 100644 --- a/internal/helper/command.go +++ b/internal/helper/command.go @@ -26,7 +26,7 @@ func (c *Command) Kill() { func GitCommandReader(ctx context.Context, args ...string) (*Command, error) { // TODO: when we switch to Go 1.7, switch to using // exec.CommandContext - return NewCommand(exec.Command("git", args...), nil, nil) + return NewCommand(CommandWrapper(ctx, "git", args...), nil, nil) } // NewCommand creates a Command from an exec.Cmd diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go new file mode 100644 index 00000000000..71cef0ac861 --- /dev/null +++ b/internal/helper/command_wrapper_go1.6.go @@ -0,0 +1,14 @@ +// +build !go1.7 + +package helper + +import ( + "os/exec" + + "golang.org/x/net/context" +) + +// CommandWrapper handles context until we compile using Go 1.7 +func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { + return exec.Command(name, arg...) +} diff --git a/internal/helper/command_wrapper_go17.go b/internal/helper/command_wrapper_go17.go new file mode 100644 index 00000000000..c381331b5f9 --- /dev/null +++ b/internal/helper/command_wrapper_go17.go @@ -0,0 +1,14 @@ +//+build go1.7 + +package helper + +import ( + "os/exec" + + "golang.org/x/net/context" +) + +// CommandWrapper handles context until we compile using Go 1.7 +func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { + return exec.CommandContext(ctx, name, arg...) +} diff --git a/internal/service/commit/isancestor.go b/internal/service/commit/isancestor.go index 3afa86fa33c..9cccd448c1d 100644 --- a/internal/service/commit/isancestor.go +++ b/internal/service/commit/isancestor.go @@ -3,7 +3,6 @@ package commit import ( "io/ioutil" "log" - "os/exec" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -30,13 +29,13 @@ func (s *server) CommitIsAncestor(ctx context.Context, in *pb.CommitIsAncestorRe return nil, grpc.Errorf(codes.InvalidArgument, message) } - ret, err := commitIsAncestorName(repoPath, in.AncestorId, in.ChildId) + ret, err := commitIsAncestorName(ctx, repoPath, in.AncestorId, in.ChildId) return &pb.CommitIsAncestorResponse{Value: ret}, err } // Assumes that `path`, `ancestorID` and `childID` are populated :trollface: -func commitIsAncestorName(path, ancestorID, childID string) (bool, error) { - osCommand := exec.Command("git", "--git-dir", path, "merge-base", "--is-ancestor", ancestorID, childID) +func commitIsAncestorName(ctx context.Context, path, ancestorID, childID string) (bool, error) { + osCommand := helper.CommandWrapper(ctx, "git", "--git-dir", path, "merge-base", "--is-ancestor", ancestorID, childID) cmd, err := helper.NewCommand(osCommand, nil, ioutil.Discard) if err != nil { return false, grpc.Errorf(codes.Internal, err.Error()) diff --git a/internal/service/smarthttp/receive_pack.go b/internal/service/smarthttp/receive_pack.go index bde99adf602..195e7321926 100644 --- a/internal/service/smarthttp/receive_pack.go +++ b/internal/service/smarthttp/receive_pack.go @@ -3,7 +3,6 @@ package smarthttp import ( "fmt" "log" - "os/exec" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -40,7 +39,7 @@ func (s *server) PostReceivePack(stream pb.SmartHTTP_PostReceivePackServer) erro log.Printf("PostReceivePack: RepoPath=%q GlID=%q", repoPath, req.GlId) - osCommand := exec.Command("git", "receive-pack", "--stateless-rpc", repoPath) + osCommand := helper.CommandWrapper(stream.Context(), "git", "receive-pack", "--stateless-rpc", repoPath) cmd, err := helper.NewCommand(osCommand, stdin, stdout, glIDEnv) if err != nil { diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go index 9528155eeb1..dc8b82df13b 100644 --- a/internal/service/smarthttp/upload_pack.go +++ b/internal/service/smarthttp/upload_pack.go @@ -2,7 +2,6 @@ package smarthttp import ( "log" - "os/exec" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -38,7 +37,7 @@ func (s *server) PostUploadPack(stream pb.SmartHTTP_PostUploadPackServer) error log.Printf("PostUploadPack: RepoPath=%q", repoPath) - osCommand := exec.Command("git", "upload-pack", "--stateless-rpc", repoPath) + osCommand := helper.CommandWrapper(stream.Context(), "git", "upload-pack", "--stateless-rpc", repoPath) cmd, err := helper.NewCommand(osCommand, stdin, stdout) if err != nil { -- GitLab From 19aabc1a95f27a76e5d6e80dc989f548471b516d Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 16:46:22 +0100 Subject: [PATCH 3/8] First stab in the dark at context cleanup --- internal/helper/command_wrapper_go1.6.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go index 71cef0ac861..e7d83e4d1f6 100644 --- a/internal/helper/command_wrapper_go1.6.go +++ b/internal/helper/command_wrapper_go1.6.go @@ -3,6 +3,7 @@ package helper import ( + "log" "os/exec" "golang.org/x/net/context" @@ -10,5 +11,15 @@ import ( // CommandWrapper handles context until we compile using Go 1.7 func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { - return exec.Command(name, arg...) + command := exec.Command(name, arg...) + + if ctx != nil { + go func() { + <-ctx.Done() + log.Printf("Context done, killing process") + command.Kill() + }() + } + + return command } -- GitLab From 624c422de65b682f7ecbd6c62b05b38ac89d368b Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 18:50:23 +0100 Subject: [PATCH 4/8] Shutdown goroutine when process is done --- internal/helper/command_wrapper_go1.6.go | 25 +++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go index e7d83e4d1f6..9643c9a9eb9 100644 --- a/internal/helper/command_wrapper_go1.6.go +++ b/internal/helper/command_wrapper_go1.6.go @@ -14,10 +14,29 @@ func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { command := exec.Command(name, arg...) if ctx != nil { + // Create a channel to listen to the command completion + done := make(chan error, 1) go func() { - <-ctx.Done() - log.Printf("Context done, killing process") - command.Kill() + done <- cmd.Wait() + }() + + // Wait for the process to shutdown or the + // context to be complete + go func() { + select { + case <-ctx.Done(): + log.Printf("Context done, killing process") + command.Kill() + + case err <- done: + if err != nil { + log.Printf("process done with error = %v", err) + } else { + log.Print("process done gracefully without error") + } + + } + }() } -- GitLab From 9f5b46288bc46172ffb75f91929a650188005fe3 Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 18:53:46 +0100 Subject: [PATCH 5/8] Fixed compiler error --- internal/helper/command_wrapper_go1.6.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go index 9643c9a9eb9..4c986b1541e 100644 --- a/internal/helper/command_wrapper_go1.6.go +++ b/internal/helper/command_wrapper_go1.6.go @@ -26,7 +26,7 @@ func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { select { case <-ctx.Done(): log.Printf("Context done, killing process") - command.Kill() + command.Process.Kill() case err <- done: if err != nil { -- GitLab From a0eb43f94a179404e00daf6d3cc8441d8afc6832 Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 19:04:22 +0100 Subject: [PATCH 6/8] Fixed another compiler fail --- internal/helper/command_wrapper_go1.6.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go index 4c986b1541e..e971be01f35 100644 --- a/internal/helper/command_wrapper_go1.6.go +++ b/internal/helper/command_wrapper_go1.6.go @@ -17,7 +17,7 @@ func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { // Create a channel to listen to the command completion done := make(chan error, 1) go func() { - done <- cmd.Wait() + done <- command.Wait() }() // Wait for the process to shutdown or the -- GitLab From e0b97db974672b9cdc3eee627a273212f9fdf852 Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 25 Apr 2017 19:18:27 +0100 Subject: [PATCH 7/8] More compile issues --- internal/helper/command_wrapper_go1.6.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go index e971be01f35..c063a9a3733 100644 --- a/internal/helper/command_wrapper_go1.6.go +++ b/internal/helper/command_wrapper_go1.6.go @@ -28,7 +28,7 @@ func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { log.Printf("Context done, killing process") command.Process.Kill() - case err <- done: + case err := <-done: if err != nil { log.Printf("process done with error = %v", err) } else { -- GitLab From 55a625bfb88e35971014dbdd1c48528e729fc553 Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Tue, 9 May 2017 06:14:00 +0100 Subject: [PATCH 8/8] Use process groups and kill the entire group on context cancel --- internal/helper/command.go | 52 +++++++++++++++++++++--- internal/helper/command_wrapper_go1.6.go | 44 -------------------- internal/helper/command_wrapper_go17.go | 14 ------- 3 files changed, 47 insertions(+), 63 deletions(-) delete mode 100644 internal/helper/command_wrapper_go1.6.go delete mode 100644 internal/helper/command_wrapper_go17.go diff --git a/internal/helper/command.go b/internal/helper/command.go index 5b9288757ba..fd10c503970 100644 --- a/internal/helper/command.go +++ b/internal/helper/command.go @@ -3,6 +3,7 @@ package helper import ( "fmt" "io" + "log" "os" "os/exec" "syscall" @@ -70,6 +71,15 @@ func NewCommand(cmd *exec.Cmd, stdin io.Reader, stdout io.Writer, env ...string) return command, nil } +func cleanUpProcessGroupNoWait(cmd *exec.Cmd) { + process := cmd.Process + if process != nil && process.Pid > 0 { + // Send SIGTERM to the process group of cmd + syscall.Kill(-process.Pid, syscall.SIGTERM) + } + +} + // CleanUpProcessGroup will send a SIGTERM signal to the process group // belonging to the `cmd` process func CleanUpProcessGroup(cmd *exec.Cmd) { @@ -77,11 +87,7 @@ func CleanUpProcessGroup(cmd *exec.Cmd) { return } - process := cmd.Process - if process != nil && process.Pid > 0 { - // Send SIGTERM to the process group of cmd - syscall.Kill(-process.Pid, syscall.SIGTERM) - } + cleanUpProcessGroupNoWait(cmd) // reap our child process cmd.Wait() @@ -101,3 +107,39 @@ func ExitStatus(err error) (int, bool) { return waitStatus.ExitStatus(), true } + +// CommandWrapper ensures that the command is executed within a context, +// and ensures that the process group is terminated with the +func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { + command := exec.Command(name, arg...) + + if ctx != nil { + // Create a channel to listen to the command completion + done := make(chan error, 1) + go func() { + done <- command.Wait() + }() + + // Wait for the process to shutdown or the + // context to be complete + go func() { + select { + case <-ctx.Done(): + log.Printf("Context done, killing process") + cleanUpProcessGroupNoWait(command) + + case err := <-done: + if err != nil { + log.Printf("process done with error = %v", err) + } else { + log.Print("process done gracefully without error") + } + cleanUpProcessGroupNoWait(command) + + } + + }() + } + + return command +} diff --git a/internal/helper/command_wrapper_go1.6.go b/internal/helper/command_wrapper_go1.6.go deleted file mode 100644 index c063a9a3733..00000000000 --- a/internal/helper/command_wrapper_go1.6.go +++ /dev/null @@ -1,44 +0,0 @@ -// +build !go1.7 - -package helper - -import ( - "log" - "os/exec" - - "golang.org/x/net/context" -) - -// CommandWrapper handles context until we compile using Go 1.7 -func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { - command := exec.Command(name, arg...) - - if ctx != nil { - // Create a channel to listen to the command completion - done := make(chan error, 1) - go func() { - done <- command.Wait() - }() - - // Wait for the process to shutdown or the - // context to be complete - go func() { - select { - case <-ctx.Done(): - log.Printf("Context done, killing process") - command.Process.Kill() - - case err := <-done: - if err != nil { - log.Printf("process done with error = %v", err) - } else { - log.Print("process done gracefully without error") - } - - } - - }() - } - - return command -} diff --git a/internal/helper/command_wrapper_go17.go b/internal/helper/command_wrapper_go17.go deleted file mode 100644 index c381331b5f9..00000000000 --- a/internal/helper/command_wrapper_go17.go +++ /dev/null @@ -1,14 +0,0 @@ -//+build go1.7 - -package helper - -import ( - "os/exec" - - "golang.org/x/net/context" -) - -// CommandWrapper handles context until we compile using Go 1.7 -func CommandWrapper(ctx context.Context, name string, arg ...string) *exec.Cmd { - return exec.CommandContext(ctx, name, arg...) -} -- GitLab