From 47b69155de9a68eb497538c106353cf5efaadc87 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Mon, 8 Apr 2024 10:11:09 +0200 Subject: [PATCH] fix: hoist signal handling from prompt confirmation Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/command/utils.go | 10 +--------- cli/command/utils_test.go | 6 +++++- cmd/docker/docker.go | 21 +++++++++++++++------ internal/test/cmd.go | 8 +++++--- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/cli/command/utils.go b/cli/command/utils.go index d7184f8c4e6a..5b2cb9721569 100644 --- a/cli/command/utils.go +++ b/cli/command/utils.go @@ -9,11 +9,9 @@ import ( "fmt" "io" "os" - "os/signal" "path/filepath" "runtime" "strings" - "syscall" "github.com/docker/cli/cli/streams" "github.com/docker/docker/api/types/filters" @@ -103,11 +101,6 @@ func PromptForConfirmation(ctx context.Context, ins io.Reader, outs io.Writer, m result := make(chan bool) - // Catch the termination signal and exit the prompt gracefully. - // The caller is responsible for properly handling the termination. - notifyCtx, notifyCancel := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM) - defer notifyCancel() - go func() { var res bool scanner := bufio.NewScanner(ins) @@ -121,8 +114,7 @@ func PromptForConfirmation(ctx context.Context, ins io.Reader, outs io.Writer, m }() select { - case <-notifyCtx.Done(): - // print a newline on termination + case <-ctx.Done(): _, _ = fmt.Fprintln(outs, "") return false, ErrPromptTerminated case r := <-result: diff --git a/cli/command/utils_test.go b/cli/command/utils_test.go index b1ea2dd74c51..1566067f3b38 100644 --- a/cli/command/utils_test.go +++ b/cli/command/utils_test.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "os" + "os/signal" "path/filepath" "strings" "syscall" @@ -135,6 +136,9 @@ func TestPromptForConfirmation(t *testing.T) { }, promptResult{false, nil}}, } { t.Run("case="+tc.desc, func(t *testing.T) { + notifyCtx, notifyCancel := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM) + t.Cleanup(notifyCancel) + buf.Reset() promptReader, promptWriter = io.Pipe() @@ -145,7 +149,7 @@ func TestPromptForConfirmation(t *testing.T) { result := make(chan promptResult, 1) go func() { - r, err := command.PromptForConfirmation(ctx, promptReader, promptOut, "") + r, err := command.PromptForConfirmation(notifyCtx, promptReader, promptOut, "") result <- promptResult{r, err} }() diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 3b3b6cb5c90f..90cd06b8ccc5 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -28,7 +28,12 @@ import ( ) func main() { - ctx := context.Background() + baseCtx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ctx, cancelNotify := signal.NotifyContext(baseCtx, platformsignals.TerminationSignals...) + defer cancelNotify() + dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx)) if err != nil { fmt.Fprintln(os.Stderr, err) @@ -223,7 +228,7 @@ func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) { }) } -func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string, envs []string) error { +func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command, subcommand string, envs []string) error { plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, subcommand, cmd) if err != nil { return err @@ -241,11 +246,15 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string, // Background signal handling logic: block on the signals channel, and // notify the plugin via the PluginServer (or signal) as appropriate. - const exitLimit = 3 - signals := make(chan os.Signal, exitLimit) - signal.Notify(signals, platformsignals.TerminationSignals...) + const exitLimit = 2 + go func() { retries := 0 + <-ctx.Done() + + signals := make(chan os.Signal, exitLimit) + signal.Notify(signals, platformsignals.TerminationSignals...) + for range signals { // If stdin is a TTY, the kernel will forward // signals to the subprocess because the shared @@ -333,7 +342,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { ccmd, _, err := cmd.Find(args) subCommand = ccmd if err != nil || pluginmanager.IsPluginCommand(ccmd) { - err := tryPluginRun(dockerCli, cmd, args[0], envs) + err := tryPluginRun(ctx, dockerCli, cmd, args[0], envs) if err == nil { if dockerCli.HooksEnabled() && dockerCli.Out().IsTerminal() && ccmd != nil { _ = pluginmanager.RunPluginHooks(dockerCli, cmd, ccmd, args[0], args) diff --git a/internal/test/cmd.go b/internal/test/cmd.go index 04df496a833b..52b44d66f1d4 100644 --- a/internal/test/cmd.go +++ b/internal/test/cmd.go @@ -3,7 +3,6 @@ package test import ( "context" "os" - "syscall" "testing" "time" @@ -32,8 +31,11 @@ func TerminatePrompt(ctx context.Context, t *testing.T, cmd *cobra.Command, cli assert.NilError(t, err) cli.SetIn(streams.NewIn(r)) + notifyCtx, notifyCancel := context.WithCancel(ctx) + t.Cleanup(notifyCancel) + go func() { - errChan <- cmd.ExecuteContext(ctx) + errChan <- cmd.ExecuteContext(notifyCtx) }() writeCtx, writeCancel := context.WithTimeout(ctx, 100*time.Millisecond) @@ -66,7 +68,7 @@ func TerminatePrompt(ctx context.Context, t *testing.T, cmd *cobra.Command, cli // sigint and sigterm are caught by the prompt // this allows us to gracefully exit the prompt with a 0 exit code - syscall.Kill(syscall.Getpid(), syscall.SIGINT) + notifyCancel() select { case <-errCtx.Done():