From a4bfd8c74499bcf0c9777a6271cc0a62a72118f8 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 18 Jun 2024 15:42:34 +0200 Subject: [PATCH 1/3] feat: add a global sigint/sigterm handler as a fallback to ctx cancellation Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cmd/docker/docker.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 3cf0ba966f..65f7499d75 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "io" "os" "os/exec" "os/signal" @@ -325,6 +326,25 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command return nil } +// registerForceExitGoroutine registers a goroutine that will force exit the +// process after 3 SIGTERM/SIGINT signals. +func registerForceExitGoroutine(ctx context.Context, w io.Writer) { + // setup a signal handler to force exit after 3 SIGTERM/SIGINT + go func() { + <-ctx.Done() + sig := make(chan os.Signal, 2) + signal.Notify(sig, platformsignals.TerminationSignals...) + count := 0 + for range sig { + count++ + if count >= 2 { + _, _ = fmt.Fprint(w, "\ngot 3 SIGTERM/SIGINTs, forcefully exiting\n") + os.Exit(1) + } + } + }() +} + //nolint:gocyclo func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { tcmd := newDockerCommand(dockerCli) @@ -384,6 +404,10 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { } } + // This is a fallback for the case where the command does not exit + // based on context cancellation. + registerForceExitGoroutine(ctx, dockerCli.Err()) + // We've parsed global args already, so reset args to those // which remain. cmd.SetArgs(args) From 1322f585fe30acd6fec33755d48a58950eba7f83 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 20 Jun 2024 14:32:38 +0200 Subject: [PATCH 2/3] test: cli force exit signal handler Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cmd/docker/docker.go | 36 +++++++------- cmd/docker/docker_test.go | 100 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 18 deletions(-) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 65f7499d75..6b8f4ab589 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -326,23 +326,23 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command return nil } -// registerForceExitGoroutine registers a goroutine that will force exit the -// process after 3 SIGTERM/SIGINT signals. -func registerForceExitGoroutine(ctx context.Context, w io.Writer) { - // setup a signal handler to force exit after 3 SIGTERM/SIGINT - go func() { - <-ctx.Done() - sig := make(chan os.Signal, 2) - signal.Notify(sig, platformsignals.TerminationSignals...) - count := 0 - for range sig { - count++ - if count >= 2 { - _, _ = fmt.Fprint(w, "\ngot 3 SIGTERM/SIGINTs, forcefully exiting\n") - os.Exit(1) - } - } - }() +// forceExitAfter3TerminationSignals waits for the first termination signal +// to be caught and the context to be marked as done, then registers a new +// signal handler for subsequent signals. It forces the process to exit +// after 3 SIGTERM/SIGINT signals. +func forceExitAfter3TerminationSignals(ctx context.Context, w io.Writer) { + // wait for the first signal to be caught and the context to be marked as done + <-ctx.Done() + // register a new signal handler for subsequent signals + sig := make(chan os.Signal, 2) + signal.Notify(sig, platformsignals.TerminationSignals...) + + // once we have received a total of 3 signals we force exit the cli + for i := 0; i < 2; i++ { + <-sig + } + _, _ = fmt.Fprint(w, "\ngot 3 SIGTERM/SIGINTs, forcefully exiting\n") + os.Exit(1) } //nolint:gocyclo @@ -406,7 +406,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { // This is a fallback for the case where the command does not exit // based on context cancellation. - registerForceExitGoroutine(ctx, dockerCli.Err()) + go forceExitAfter3TerminationSignals(ctx, dockerCli.Err()) // We've parsed global args already, so reset args to those // which remain. diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index 8fc7ade3a4..c7e6952e2b 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -1,17 +1,28 @@ package main import ( + "bufio" "bytes" "context" + "fmt" "io" "os" + "os/exec" + "os/signal" + "strings" + "syscall" "testing" + "time" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/debug" + "github.com/docker/cli/cli/streams" + "github.com/docker/cli/cmd/docker/internal/signals" "github.com/sirupsen/logrus" + "github.com/spf13/cobra" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" + "gotest.tools/v3/poll" ) func TestClientDebugEnabled(t *testing.T) { @@ -75,3 +86,92 @@ func TestVersion(t *testing.T) { assert.NilError(t, err) assert.Check(t, is.Contains(b.String(), "Docker version")) } + +func TestFallbackForceExit(t *testing.T) { + longRunningCommand := cobra.Command{ + RunE: func(cmd *cobra.Command, args []string) error { + read, _, err := os.Pipe() + if err != nil { + return err + } + + // wait until the parent process sends a signal to exit + _, _, err = bufio.NewReader(read).ReadLine() + return err + }, + } + + // This is the child process that will run the long running command + if os.Getenv("TEST_FALLBACK_FORCE_EXIT") == "1" { + fmt.Println("running long command") + ctx, cancel := signal.NotifyContext(context.Background(), signals.TerminationSignals...) + t.Cleanup(cancel) + + longRunningCommand.SetErr(streams.NewOut(os.Stderr)) + longRunningCommand.SetOut(streams.NewOut(os.Stdout)) + + go forceExitAfter3TerminationSignals(ctx, streams.NewOut(os.Stderr)) + + err := longRunningCommand.ExecuteContext(ctx) + if err != nil { + os.Exit(0) + } + return + } + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + // spawn the child process + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestFallbackForceExit") + cmd.Env = append(os.Environ(), "TEST_FALLBACK_FORCE_EXIT=1") + + var buf strings.Builder + cmd.Stderr = &buf + cmd.Stdout = &buf + + t.Cleanup(func() { + _ = cmd.Process.Kill() + }) + + assert.NilError(t, cmd.Start()) + + poll.WaitOn(t, func(t poll.LogT) poll.Result { + if strings.Contains(buf.String(), "running long command") { + return poll.Success() + } + return poll.Continue("waiting for child process to start") + }, poll.WithTimeout(1*time.Second), poll.WithDelay(100*time.Millisecond)) + + for i := 0; i < 3; i++ { + cmd.Process.Signal(syscall.SIGINT) + time.Sleep(100 * time.Millisecond) + } + + cmdErr := make(chan error, 1) + go func() { + cmdErr <- cmd.Wait() + }() + + poll.WaitOn(t, func(t poll.LogT) poll.Result { + if strings.Contains(buf.String(), "got 3 SIGTERM/SIGINTs, forcefully exiting") { + return poll.Success() + } + return poll.Continue("waiting for child process to exit") + }, + poll.WithTimeout(1*time.Second), poll.WithDelay(100*time.Millisecond)) + + select { + case cmdErr := <-cmdErr: + assert.Error(t, cmdErr, "exit status 1") + exitErr, ok := cmdErr.(*exec.ExitError) + if !ok { + t.Fatalf("unexpected error type: %T", cmdErr) + } + if exitErr.Success() { + t.Fatalf("unexpected exit status: %v", exitErr) + } + case <-time.After(1 * time.Second): + t.Fatal("timed out waiting for child process to exit") + } +} From faf7647dcf3a43faa92bf7fceacef7a41f4e525b Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 20 Jun 2024 16:56:24 +0200 Subject: [PATCH 3/3] refactor: remove test Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cmd/docker/docker_test.go | 100 -------------------------------------- 1 file changed, 100 deletions(-) diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index c7e6952e2b..8fc7ade3a4 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -1,28 +1,17 @@ package main import ( - "bufio" "bytes" "context" - "fmt" "io" "os" - "os/exec" - "os/signal" - "strings" - "syscall" "testing" - "time" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/debug" - "github.com/docker/cli/cli/streams" - "github.com/docker/cli/cmd/docker/internal/signals" "github.com/sirupsen/logrus" - "github.com/spf13/cobra" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" - "gotest.tools/v3/poll" ) func TestClientDebugEnabled(t *testing.T) { @@ -86,92 +75,3 @@ func TestVersion(t *testing.T) { assert.NilError(t, err) assert.Check(t, is.Contains(b.String(), "Docker version")) } - -func TestFallbackForceExit(t *testing.T) { - longRunningCommand := cobra.Command{ - RunE: func(cmd *cobra.Command, args []string) error { - read, _, err := os.Pipe() - if err != nil { - return err - } - - // wait until the parent process sends a signal to exit - _, _, err = bufio.NewReader(read).ReadLine() - return err - }, - } - - // This is the child process that will run the long running command - if os.Getenv("TEST_FALLBACK_FORCE_EXIT") == "1" { - fmt.Println("running long command") - ctx, cancel := signal.NotifyContext(context.Background(), signals.TerminationSignals...) - t.Cleanup(cancel) - - longRunningCommand.SetErr(streams.NewOut(os.Stderr)) - longRunningCommand.SetOut(streams.NewOut(os.Stdout)) - - go forceExitAfter3TerminationSignals(ctx, streams.NewOut(os.Stderr)) - - err := longRunningCommand.ExecuteContext(ctx) - if err != nil { - os.Exit(0) - } - return - } - - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - - // spawn the child process - cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestFallbackForceExit") - cmd.Env = append(os.Environ(), "TEST_FALLBACK_FORCE_EXIT=1") - - var buf strings.Builder - cmd.Stderr = &buf - cmd.Stdout = &buf - - t.Cleanup(func() { - _ = cmd.Process.Kill() - }) - - assert.NilError(t, cmd.Start()) - - poll.WaitOn(t, func(t poll.LogT) poll.Result { - if strings.Contains(buf.String(), "running long command") { - return poll.Success() - } - return poll.Continue("waiting for child process to start") - }, poll.WithTimeout(1*time.Second), poll.WithDelay(100*time.Millisecond)) - - for i := 0; i < 3; i++ { - cmd.Process.Signal(syscall.SIGINT) - time.Sleep(100 * time.Millisecond) - } - - cmdErr := make(chan error, 1) - go func() { - cmdErr <- cmd.Wait() - }() - - poll.WaitOn(t, func(t poll.LogT) poll.Result { - if strings.Contains(buf.String(), "got 3 SIGTERM/SIGINTs, forcefully exiting") { - return poll.Success() - } - return poll.Continue("waiting for child process to exit") - }, - poll.WithTimeout(1*time.Second), poll.WithDelay(100*time.Millisecond)) - - select { - case cmdErr := <-cmdErr: - assert.Error(t, cmdErr, "exit status 1") - exitErr, ok := cmdErr.(*exec.ExitError) - if !ok { - t.Fatalf("unexpected error type: %T", cmdErr) - } - if exitErr.Success() { - t.Fatalf("unexpected exit status: %v", exitErr) - } - case <-time.After(1 * time.Second): - t.Fatal("timed out waiting for child process to exit") - } -}