From 5f221783c2a5791ec34e4070353a3125fd0847c9 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 5 Dec 2024 13:11:17 +0100 Subject: [PATCH 1/2] feat: cause on user-terminated context Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cmd/docker/docker.go | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 29a3ed595755..0d190cbe2ad5 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -28,16 +28,43 @@ import ( "go.opentelemetry.io/otel" ) +var errCtxUserTerminated = errors.New("user terminated the process") + func main() { - err := dockerMain(context.Background()) - if err != nil && !errdefs.IsCancelled(err) { + ctx := context.Background() + err := dockerMain(ctx) + if err != nil && !errdefs.IsCancelled(err) && !errors.Is(err, errCtxUserTerminated) { _, _ = fmt.Fprintln(os.Stderr, err) os.Exit(getExitCode(err)) } } +func notifyContext(ctx context.Context, signals ...os.Signal) (context.Context, context.CancelFunc) { + ch := make(chan os.Signal, 1) + signal.Notify(ch, signals...) + + ctx, cancel := context.WithCancelCause(ctx) + + go func() { + select { + case <-ctx.Done(): + signal.Stop(ch) + return + case <-ch: + cancel(errCtxUserTerminated) + signal.Stop(ch) + return + } + }() + + return ctx, func() { + signal.Stop(ch) + cancel(nil) + } +} + func dockerMain(ctx context.Context) error { - ctx, cancelNotify := signal.NotifyContext(ctx, platformsignals.TerminationSignals...) + ctx, cancelNotify := notifyContext(ctx, platformsignals.TerminationSignals...) defer cancelNotify() dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx)) From a20d0c8212a1681896940c858254b52c6ecae61e Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 3 Dec 2024 14:00:29 +0100 Subject: [PATCH 2/2] cli/command: ctx cancel should not print or produce a non zero exit code The user might kill the CLI through a SIGINT/SIGTERM which cancels the main context we pass around. Currently the context cancel error is printed alongside any other wrapped error with a generic exit code (125). This patch improves on this behavior and prevents any error from being printed when they match `context.Cancelled`. The `cli.StatusError` error would wrap errors but not provide a way to unwrap them. This would lead to situations where `errors.Is` would not match the underlying error. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/cobra.go | 2 +- cli/command/config/inspect.go | 2 +- cli/command/container/create.go | 6 +++--- cli/command/container/run.go | 12 ++++++------ cli/command/container/run_test.go | 8 ++++---- cli/command/image/build.go | 2 +- cli/command/inspect/inspector.go | 4 ++-- cli/command/node/inspect.go | 2 +- cli/command/secret/inspect.go | 2 +- cli/command/service/inspect.go | 2 +- cli/command/system/events.go | 2 +- cli/command/system/info.go | 2 +- cli/command/system/version.go | 2 +- cli/error.go | 13 ++++++++++--- 14 files changed, 34 insertions(+), 27 deletions(-) diff --git a/cli/cobra.go b/cli/cobra.go index feab2b4a91c8..631c9f61d697 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -93,7 +93,7 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error { } return StatusError{ - Status: fmt.Sprintf("%s\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()), + Cause: fmt.Errorf("%w\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()), StatusCode: 125, } } diff --git a/cli/command/config/inspect.go b/cli/command/config/inspect.go index 7ae4a4c40435..e2f4fe6b210c 100644 --- a/cli/command/config/inspect.go +++ b/cli/command/config/inspect.go @@ -67,7 +67,7 @@ func RunConfigInspect(ctx context.Context, dockerCli command.Cli, opts InspectOp } if err := InspectFormatWrite(configCtx, opts.Names, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 9f73b7f0afe3..863440a2d773 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -93,7 +93,7 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command { func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error { if err := validatePullOpt(options.pull); err != nil { return cli.StatusError{ - Status: withHelp(err, "create").Error(), + Cause: withHelp(err, "create"), StatusCode: 125, } } @@ -110,13 +110,13 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType) if err != nil { return cli.StatusError{ - Status: withHelp(err, "create").Error(), + Cause: withHelp(err, "create"), StatusCode: 125, } } if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil { return cli.StatusError{ - Status: withHelp(err, "create").Error(), + Cause: withHelp(err, "create"), StatusCode: 125, } } diff --git a/cli/command/container/run.go b/cli/command/container/run.go index bb0340ed1b49..7fc5fb47340a 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -85,7 +85,7 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command { func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error { if err := validatePullOpt(ropts.pull); err != nil { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Cause: withHelp(err, "run"), StatusCode: 125, } } @@ -103,13 +103,13 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro // just in case the parse does not exit if err != nil { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Cause: withHelp(err, "run"), StatusCode: 125, } } if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Cause: withHelp(err, "run"), StatusCode: 125, } } @@ -315,20 +315,20 @@ func toStatusError(err error) error { if strings.Contains(errMsg, "executable file not found") || strings.Contains(errMsg, "no such file or directory") || strings.Contains(errMsg, "system cannot find the file specified") { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Cause: withHelp(err, "run"), StatusCode: 127, } } if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Cause: withHelp(err, "run"), StatusCode: 126, } } return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Cause: withHelp(err, "run"), StatusCode: 125, } } diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 81b176d904c9..c48ba9415649 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -294,10 +294,10 @@ func TestRunPullTermination(t *testing.T) { select { case cmdErr := <-cmdErrC: - assert.Equal(t, cmdErr, cli.StatusError{ - StatusCode: 125, - Status: "docker: context canceled\n\nRun 'docker run --help' for more information", - }) + assert.ErrorIs(t, cmdErr, context.Canceled) + v, ok := cmdErr.(cli.StatusError) + assert.Check(t, ok) + assert.Check(t, is.Equal(v.StatusCode, 125)) case <-time.After(10 * time.Second): t.Fatal("cmd did not return before the timeout") } diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 4a258646dcce..3acb4b8151f7 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -371,7 +371,7 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) if options.quiet { fmt.Fprintf(dockerCli.Err(), "%s%s", progBuff, buildBuff) } - return cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code} + return cli.StatusError{Cause: jerr, StatusCode: jerr.Code} } return err } diff --git a/cli/command/inspect/inspector.go b/cli/command/inspect/inspector.go index 46ba8750d6a4..ed2d2f3a02ae 100644 --- a/cli/command/inspect/inspector.go +++ b/cli/command/inspect/inspector.go @@ -67,7 +67,7 @@ type GetRefFunc func(ref string) (any, []byte, error) func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFunc) error { inspector, err := NewTemplateInspectorFromString(out, tmplStr) if err != nil { - return cli.StatusError{StatusCode: 64, Status: err.Error()} + return cli.StatusError{StatusCode: 64, Cause: err} } var inspectErrs []string @@ -90,7 +90,7 @@ func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFu if len(inspectErrs) != 0 { return cli.StatusError{ StatusCode: 1, - Status: strings.Join(inspectErrs, "\n"), + Cause: errors.New(strings.Join(inspectErrs, "\n")), } } return nil diff --git a/cli/command/node/inspect.go b/cli/command/node/inspect.go index 270a14bd2bdd..fe613b9b0ea9 100644 --- a/cli/command/node/inspect.go +++ b/cli/command/node/inspect.go @@ -69,7 +69,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } if err := InspectFormatWrite(nodeCtx, opts.nodeIds, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/secret/inspect.go b/cli/command/secret/inspect.go index 5559d43d383e..f3661fe87dd7 100644 --- a/cli/command/secret/inspect.go +++ b/cli/command/secret/inspect.go @@ -65,7 +65,7 @@ func runSecretInspect(ctx context.Context, dockerCli command.Cli, opts inspectOp } if err := InspectFormatWrite(secretCtx, opts.names, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/service/inspect.go b/cli/command/service/inspect.go index 28b957f77310..2a948abba60e 100644 --- a/cli/command/service/inspect.go +++ b/cli/command/service/inspect.go @@ -94,7 +94,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } if err := InspectFormatWrite(serviceCtx, opts.refs, getRef, getNetwork); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/system/events.go b/cli/command/system/events.go index 9b5f965e5b3c..7e7055a3bd5d 100644 --- a/cli/command/system/events.go +++ b/cli/command/system/events.go @@ -60,7 +60,7 @@ func runEvents(ctx context.Context, dockerCli command.Cli, options *eventsOption if err != nil { return cli.StatusError{ StatusCode: 64, - Status: "Error parsing format: " + err.Error(), + Cause: fmt.Errorf("error parsing format: %w", err), } } ctx, cancel := context.WithCancel(ctx) diff --git a/cli/command/system/info.go b/cli/command/system/info.go index 108a65a5f1f4..84b1ccdbb4a1 100644 --- a/cli/command/system/info.go +++ b/cli/command/system/info.go @@ -459,7 +459,7 @@ func formatInfo(output io.Writer, info dockerInfo, format string) error { if err != nil { return cli.StatusError{ StatusCode: 64, - Status: "template parsing error: " + err.Error(), + Cause: fmt.Errorf("template parsing error: %w", err), } } err = tmpl.Execute(output, info) diff --git a/cli/command/system/version.go b/cli/command/system/version.go index f25446b06dd8..69ca54e40060 100644 --- a/cli/command/system/version.go +++ b/cli/command/system/version.go @@ -148,7 +148,7 @@ func runVersion(ctx context.Context, dockerCli command.Cli, opts *versionOptions var err error tmpl, err := newVersionTemplate(opts.format) if err != nil { - return cli.StatusError{StatusCode: 64, Status: err.Error()} + return cli.StatusError{StatusCode: 64, Cause: err} } // TODO print error if kubernetes is used? diff --git a/cli/error.go b/cli/error.go index 8c4a5f952bce..abe65c6225ab 100644 --- a/cli/error.go +++ b/cli/error.go @@ -6,7 +6,7 @@ import ( // StatusError reports an unsuccessful exit by a command. type StatusError struct { - Status string + Cause error StatusCode int } @@ -14,8 +14,15 @@ type StatusError struct { // it is returned as-is, otherwise it generates a generic error-message // based on the StatusCode. func (e StatusError) Error() string { - if e.Status == "" { + if e.Cause == nil { return "exit status " + strconv.Itoa(e.StatusCode) } - return e.Status + return e.Cause.Error() +} + +// Unwrap returns the wrapped error. +// +// This allows StatusError to be checked with errors.Is. +func (e StatusError) Unwrap() error { + return e.Cause }