Don't print "context canceled" if user terminated #5778

laurazard · 2025-01-29T16:16:48Z

fixes don't print "context canceled" errors when canceling an action (CTRL-C) #5659

- What I did

Alternative to/supercedes #5666.

Without breaking API compatibility, this patch allows us to know whether a returned cli/StatusError was caused by a context cancellation or not, which we can use to provide a nicer UX and not print the Go "context canceled" error message if this is the cause.

This change is safe and does not affect any caller that does not set Cause with the root error.

- How I did it

(This is basically what I suggested in #5666 – see #5666 (comment), and #5666 (comment) – and also what @ndeloof suggested – I figured it would be easier to just implement it since I think most of the maintainers are in agreement about this, and don't want to unexport cli.StatusError )

By adding a Cause field to the StatusError type, and implementing Unwrap so that StatusError can be checked with errors.Is.

We can deprecate Status later, or we can keep it if we want to let callers set a nicer error message (and always use that if it is set).

- How to verify it
docker run --pull=always ubuntu echo "hi" and cancel during the pull.

Before:

/docker run --pull=always ubuntu echo "hi"
latest: Pulling from library/ubuntu
8bb55f067777: Pulling fs layer
^Cdocker: context canceled

Run 'docker run --help' for more information

With the change:

 ./build/docker run --pull=always ubuntu echo "hi"
latest: Pulling from library/ubuntu
8bb55f067777: Downloading [=====================>                             ]  12.45MB/28.89MB
^C

- Description for the changelog

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

codecov-commenter · 2025-01-29T16:20:35Z

Codecov Report

Attention: Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 59.41%. Comparing base (bdd70c1) to head (ccaa2e0).

❌ Your patch status has failed because the patch coverage (8.33%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5778      +/-   ##
==========================================
- Coverage   59.42%   59.41%   -0.02%     
==========================================
  Files         347      347              
  Lines       29402    29410       +8     
==========================================
+ Hits        17472    17473       +1     
- Misses      10958    10965       +7     
  Partials      972      972

Without breaking API compatibility, this patch allows us to know whether a returned `cli/StatusError` was caused by a context cancellation or not, which we can use to provide a nicer UX and not print the Go "context canceled" error message if this is the cause. Signed-off-by: Laura Brehm <[email protected]>

thaJeztah · 2025-01-29T17:16:44Z

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) && !errors.Is(err, context.Canceled) {


Curious; errdefs.IsCancelled doesn't detect these? (that sounds like a possible bug if it doesn't)

It doesn't, no. It checks that it implements the errdefs cancelled interface, not for context.Canceled.

// ErrCancelled signals that the action was cancelled. type ErrCancelled interface { Cancelled() }

But don't we want to be specific to only ignoring the user cancelled the process vs something/someone cancelled the process? AFAIK context.Canceled can be a multitude of things and not just user cancelled.

See #5760

What else could cancel it though?

Anything using context.WithCancel() can call the cancel func inside its own logic and return ctx.Err(). Since the error is ambiguous and can be used by anything down the call stack I'd prefer to impose a check on an error we know of. To me it seems it would be better to have something print out context canceled in a case where we don't properly handle it than every occurrence of context.Canceled silenced

I'm okay with cherry-picking some of #5760 and using that to filter. Does that sound good @vvoland @thaJeztah?

To be honest, I'd be inclined to consider function accepting ctx and returning context.Canceled when the input ctx.Err() == nil as buggy.

A logic using context.WithCancel should be able to handle that in its own boundary.

It is fine for a function to handle its own login with context.WithCancel. I'm just saying when it returns an error and we check it all the way at the entry point of the program, then it shouldn't be silenced automatically.

thaJeztah · 2025-01-29T17:17:34Z

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) && !errors.Is(err, context.Canceled) {
 		_, _ = fmt.Fprintln(os.Stderr, err)
 		os.Exit(getExitCode(err))


I guess existing code didn't do so, but was wondering if the exit code (of explicitly set) should still be regarded (not sure)

I don't think so – in the past I think we've actually reviewed things to not exit with an erroneous exit code in some instances. We could take a look and see what other tools do though, and what makes sense. @tianon thoughts?

I think I'm missing some nuance to the question 😅 ❤️

In general, I think the case described (Ctrl+C to cancel the pull during docker run --pull) should have a non-zero exit code for sure (when cancelled, the command didn't complete the request successfully).

For example:

$ sleep 100 ^C $ echo $? 130

(but this feels too obvious, hence my thought that I must be missing some question nuance 😂)

Nooo, you're definitely right. I was just wondering if there were some weird edge cases to this (if a user cancels mid confirmation prompt for example), or if you had some other thoughts. You're my goto for "I wonder if there's any weird expectations/precedent for [insert Unixy thing]".
I know at least for the CLI we used to have that behavior:

$ docker login -u bork registry-1.docker.io Password: [^C] $ echo $? 0

IMO, in general a canceled operation should have a non-zero exit code, BUT with some exceptions, for example:

docker stats by design never ends by itself, so terminating it with ctrl+c should produce 0 exit code.
However, ctrl+c docker stats --no-stream before it finished by itself should produce a non-zero exit code.

$ docker login -u bork registry-1.docker.io Password: [^C] $ echo $? 0

Feels like it should fall under the general rule of non-zero exit code.

My reasoning is, that if you execute (with terminal attached) a script like:

set -x docker login -u bork registry-1.docker.io # do something after user logged in docker pull ... ...

Then docker login which was ctrl+c'd should exit with a non-zero exit code so the script can exit instead of continuing execution.

I definitely agree. Even for the docker stats case, one could argue that the expectation is that it indeed stays attached, and any exit (even user caused) is something that merits an exit code. Looking at tail,

$ tail -F bork.txt hi! [...] ^C $ echo $? 130

Hmm, that's also a fair point!

In general, I think the case described (Ctrl+C to cancel the pull during docker run --pull) should have a non-zero exit code for sure (when cancelled, the command didn't complete the request successfully).

Ah, yes, that also makes sense; I recall I was in doubt what would be correct when I wrote the ticket #5659;

I also noticed that the exit-code is non-zero in these situations, but I'm not sure what the correct thing to do is there. For example, tail also exits with a non-zero exit-code, although it uses a specific code for it (130);

So I wasn't sure what the correct way was to look at it;

User cancelled the pull, and we cancelled, so cancelling "succeeded successfully"

docker pull was cancelled which happened to be "by user" (in my example), but the pull didn't complete (so "unclean" shutdown of the pull)

But, obviously, that was from my perspective "I cancelled the pull", but it may not be limited to the user taking that action, and (for example) if the cancel happened as part of a script, then having a non-zero exit code would make a lot of sense (something happened, and the pull is not done!).

And now I recall we made a similar mistake on docker ps --filter XXX and docker image ls --filter XXX, where we considered "filtering succeeded, but didn't return anything" to be "success", which made sense when we did, but later bit us in the behind, because when scripting, it WILL be beneficial to know "no results, so we may want to take alternative action"

fayazkhan121 · 2025-01-31T14:13:01Z

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) && !errors.Is(err, context.Canceled) {


This improves clarity by reducing unnecessary negations.

laurazard requested review from ndeloof, thaJeztah and vvoland January 29, 2025 16:16

laurazard self-assigned this Jan 29, 2025

laurazard force-pushed the add-cause-statuserr branch from e98f488 to ccaa2e0 Compare January 29, 2025 16:35

vvoland added kind/enhancement status/2-code-review area/ux labels Jan 29, 2025

vvoland added this to the 28.0.0 milestone Jan 29, 2025

thaJeztah reviewed Jan 29, 2025

View reviewed changes

ndeloof approved these changes Jan 29, 2025

View reviewed changes

ndeloof mentioned this pull request Jan 29, 2025

cli: deprecate cli.StatusError direct field usage #5666

Closed

fayazkhan121 approved these changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't print "context canceled" if user terminated #5778

Don't print "context canceled" if user terminated #5778

laurazard commented Jan 29, 2025 •

edited by thaJeztah

Loading

codecov-commenter commented Jan 29, 2025 •

edited

Loading

thaJeztah Jan 29, 2025

laurazard Jan 29, 2025

Benehiko Jan 30, 2025

vvoland Jan 30, 2025

Benehiko Jan 30, 2025

laurazard Jan 30, 2025

vvoland Jan 30, 2025

Benehiko Jan 30, 2025

thaJeztah Jan 29, 2025

laurazard Jan 29, 2025

tianon Jan 29, 2025

tianon Jan 29, 2025

laurazard Jan 30, 2025 •

edited

Loading

vvoland Jan 30, 2025

vvoland Jan 30, 2025

laurazard Jan 30, 2025

vvoland Jan 30, 2025

thaJeztah Jan 30, 2025 •

edited

Loading

fayazkhan121 Jan 31, 2025

Don't print "context canceled" if user terminated #5778

Are you sure you want to change the base?

Don't print "context canceled" if user terminated #5778

Conversation

laurazard commented Jan 29, 2025 • edited by thaJeztah Loading

codecov-commenter commented Jan 29, 2025 • 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

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 Jan 30, 2025 • 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

thaJeztah Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard commented Jan 29, 2025 •

edited by thaJeztah

Loading

codecov-commenter commented Jan 29, 2025 •

edited

Loading

laurazard Jan 30, 2025 •

edited

Loading

thaJeztah Jan 30, 2025 •

edited

Loading