cli/command: ctx cancel should not print or produce a non zero exit code #5666

Benehiko · 2024-12-03T13:09:01Z

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.

Closes #5659

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2024-12-03T13:11:02Z

Codecov Report

Attention: Patch coverage is 14.89362% with 40 lines in your changes missing coverage. Please review.

Project coverage is 59.49%. Comparing base (e7078bf) to head (a20d0c8).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5666      +/-   ##
==========================================
- Coverage   59.53%   59.49%   -0.05%     
==========================================
  Files         346      346              
  Lines       29356    29378      +22     
==========================================
  Hits        17477    17477              
- Misses      10909    10931      +22     
  Partials      970      970

laurazard · 2024-12-03T13:49:44Z

cli/error.go

@@ -6,16 +6,22 @@ import (

 // StatusError reports an unsuccessful exit by a command.
 type StatusError struct {
-	Status     string
+	Status     error


If we're changing this, it would make more sense to do

Suggested change

Status error

StatusError error

or

Suggested change

Status error

Cause error

However, we need to use some caution changing this. It's widely used, so we could never include this in a minor release, and even in a major we need to be careful.

It might be better to fix this without changing cli.StatusError first, and do this change separately, unless we absolutely need it to fix this.

I'll think a bit about a solution for this, but I don't think we should concern ourselves too much with externals importing this particular package.

but I don't think we should concern ourselves too much with externals importing this particular package.

For any reason in particular?

AFAIK we have no SLA stating that internal packages in the CLI should never break due to external usage. I'd take the side of our duties are to Docker and Docker's users before a 3rd party using our code.

I don't agree. If this was an API client, then sure. If this was a behavior change of a feature of the CLI, then sure. But this is an error type used within the CLI, so I don't agree with your assessment. Again our duties are first towards Docker and Docker's users. Since this change is beneficial to both it outweighs any risk we perceive to third parties.

We cannot allow ourselves to be blocked by, in my opinion, an inconsequential risk. Breaking the error type just simply means the third party has to change it when they update.

The impact is also anecdotal and doesn't necessarily convey the actual effect since the GitHub search only tells half the story. We honestly don't even know if there will be a risk to those third party projects as we don't keep track of them 🤷

It's an exported type, so it's part of the API. That isn't an opinion to agree or disagree on, it's a fact.

Since this change is beneficial to both it outweighs any risk we perceive to third parties.

I'll reiterate since maybe I wasn't clear:

It might be better to fix this without changing cli.StatusError first, and do this change separately, unless we absolutely need it to fix this.

I'm not saying we can't ever change the API, we often do. I'm saying that breaking the API has consequences: what kind of releases we can ship it in (major vs. minor), more work for consumers, etc., and if we can do fix this now without changing the API and hold off on the change we should.

This means that if there is an option that allows us to get that benefit to our users without the negatives, then we should prefer that over the alternatives. Not that we would

allow ourselves to be blocked by, in my opinion, an inconsequential risk.

As an aside, working on large, old codebases with decades of consumers (who we benefit from integrating with us) requires considering all the downstream effects of your changes, and consider the best/least disruptive way to achieve them.

Getting your changes merged also requires that you take a constructive/cooperative approach to discussions and other's perspectives and not simply disregard other's opinions.

AFAIK we have no SLA stating that internal packages in the CLI should never break due to external usage. I'd take the side of our duties are to Docker and Docker's users before a 3rd party using our code.

We do. The contract is "go modules", which requires following SemVer; we do tend to break that contract (too often), because we don't have a go.mod yet (thus no vehicle to ship "major" release, which requires a rename of the module (/v28/...), but we should not ship breaking changes in a minor or patch release.

We don't want to break trust there, because doing so means that other code may decide to never update, and now we're stuck with potentially many projects using legacy code (or being broken), which is not great.

There may be some exceptions where we have no choice, but if there's ways to make changes without that resulting in a breaking change, we should.

I understand where you are coming from, but I want to clarify my position. Docker as an Organization does not have an agreement with third parties to consume the CLI code (afaik).

Having an implicit SLA through Go's module specification for the CLI seems counter-intuitive to me as the CLI is not built as a library to be consumed.

With this implicit SLA contributing anything, however small (as in this case), will always get blocked or pushed out to a major release, which I see as counterproductive.

With this implicit SLA contributing anything, however small (as in this case), will always get blocked or pushed out to a major release, which I see as counterproductive.

This is simply not true. There are various ways to contribute changes that don't break semver API contracts.

Having an implicit SLA through Go's module specification for the CLI seems counter-intuitive to me as the CLI is not built as a library to be consumed.

So your proposal is that we simply disregard Semver/Go Modules, and ship breaking API changes in minor/patch releases? If everyone started doing this, everything in the Go Ecosystem would break due to the toolchain no longer being able to depend on guarantees such as these when selecting versions, updating any dependencies would become a chore, pipelines would break overnight, etc.

That's discounting the fact that, again, we benefit (and want) people to build things using our packages, and doing that requires building trust on the stability of those packages. Some packages we can (and should) move/deprecate (on major releases) to /internal if we don't want them being used (and we have done this), but as long as they're explicitly exported we must uphold the expectations we subscribed to when we shipped exported packages. Breaking this wouldn't even be okay from an internal API consumer's point of view.

laurazard · 2024-12-03T13:53:05Z

cmd/docker/docker.go

@@ -30,7 +30,7 @@ import (

 func main() {
 	err := dockerMain(context.Background())
-	if err != nil && !errdefs.IsCancelled(err) {
+	if err != nil && !errdefs.IsCancelled(err) && !errdefs.IsContext(err) {


Why the !errdefs.IsContext(err)? IMO I'd like to see a message telling me what happened if a request timed out or some such.

true, right now I don't want to print any exit status or error message when the context is cancelled. I think it might be possible to determine if this is a sigint/sigterm vs a timeout for example.

thaJeztah · 2024-12-03T13:57:12Z

cli/command/container/run.go

@@ -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(),
+			Status:     withHelp(err, "run"),


Perhaps as an intermediate (non-breaking) step, we could;

change withHelp to return a string (instead of error)

make withHelp "context-aware" and discard the error-message if it's a context error

(Haven't looked in-depth if that's an option, so take that with a grain of salt 😅)

We could not try to handle this all magically, and instead handle context cancellation errors at each place where we think they occur. That would have the benefit of not hiding unexpected context cancelled errors if they're coming from somewhere a bug/an unexpected interaction. DRI is fine, but too much generalization can also be bad.

make withHelp "context-aware" and discard the error-message if it's a context error

we could, but this would mean inconsistent behavior, some places would print context cancelled and others nothing.

laurazard · 2024-12-03T14:01:57Z

cli/error.go

+// Unwrap allows wrapped errors stored in StatusError to be checked
+// using `errors.Is`.


Unwrap doesn't allow wrapped errors to be checked using errors.Is – it allows StatusError to be checked with errors.Is (and exposes the underlying error).

Suggested change

// Unwrap allows wrapped errors stored in StatusError to be checked

// using `errors.Is`.

// Unwrap returns the wrapped error.

//

// This allows StatusError to be checked with errors.Is.

True, what I tried to say was: errors.Is checks for errors down the error chain. Since we wrap errors using fmt.Errorf("%w", err) and store that inside StatusError we need to check the underlying error instead of just err == StatusError.

Right, it's just a matter of wording. IMO the GoDoc for Unwrap should explain what (from the perspective of StatusError) what Unwrap does, which specifically is "return the wrapped error – which allows checking StatusErr with errors.Is".

allows wrapped errors stored in StatusError to be checked using errors.Is. is less clear to me since it could also be interpreted as "allows you to do errors.Is on the underlying error" which isn't the case.

Signed-off-by: Alano Terblanche <[email protected]>

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 <[email protected]>

Benehiko force-pushed the ctx-cancel-status branch from b6c8353 to d7c81a3 Compare December 3, 2024 13:14

laurazard reviewed Dec 3, 2024

View reviewed changes

thaJeztah reviewed Dec 3, 2024

View reviewed changes

laurazard reviewed Dec 3, 2024

View reviewed changes

Benehiko added 2 commits December 5, 2024 13:48

feat: cause on user-terminated context

5f22178

Signed-off-by: Alano Terblanche <[email protected]>

Benehiko force-pushed the ctx-cancel-status branch from 9275a27 to a20d0c8 Compare December 5, 2024 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli/command: ctx cancel should not print or produce a non zero exit code #5666

cli/command: ctx cancel should not print or produce a non zero exit code #5666

Benehiko commented Dec 3, 2024 •

edited

Loading

codecov-commenter commented Dec 3, 2024 •

edited

Loading

laurazard Dec 3, 2024

laurazard Dec 3, 2024

Benehiko Dec 3, 2024

laurazard Dec 3, 2024

Benehiko Dec 3, 2024

Benehiko Dec 4, 2024

laurazard Dec 4, 2024 •

edited

Loading

thaJeztah Dec 4, 2024

Benehiko Dec 5, 2024

laurazard Dec 5, 2024

laurazard Dec 3, 2024

Benehiko Dec 3, 2024

thaJeztah Dec 3, 2024

laurazard Dec 3, 2024

Benehiko Dec 3, 2024

laurazard Dec 3, 2024

Benehiko Dec 3, 2024

laurazard Dec 3, 2024

		// Unwrap allows wrapped errors stored in StatusError to be checked
		// using `errors.Is`.

cli/command: ctx cancel should not print or produce a non zero exit code #5666

Are you sure you want to change the base?

cli/command: ctx cancel should not print or produce a non zero exit code #5666

Conversation

Benehiko commented Dec 3, 2024 • edited Loading

codecov-commenter commented Dec 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko commented Dec 3, 2024 •

edited

Loading

codecov-commenter commented Dec 3, 2024 •

edited

Loading

laurazard Dec 4, 2024 •

edited

Loading