Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Benehiko
Copy link
Member

@Benehiko Benehiko commented Dec 3, 2024

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
Copy link

codecov-commenter commented Dec 3, 2024

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              

cli/error.go Outdated
@@ -6,16 +6,22 @@ import (

// StatusError reports an unsuccessful exit by a command.
type StatusError struct {
Status string
Status error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

For any reason in particular?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Member

@laurazard laurazard Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cli/error.go Outdated
Comment on lines 23 to 24
// Unwrap allows wrapped errors stored in StatusError to be checked
// using `errors.Is`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <18033717+Benehiko@users.noreply.github.com>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

don't print "context canceled" errors when canceling an action (CTRL-C)
4 participants