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.

Sign up for GitHub

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+16 −4
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cli/command/container/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,19 +324,22 @@ 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{
Cause: err,
Status: withHelp(err, "run").Error(),
StatusCode: 127,
}
}

if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) {
return cli.StatusError{
Cause: err,
Status: withHelp(err, "run").Error(),
StatusCode: 126,
}
}

return cli.StatusError{
Cause: err,
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
Expand Down
1 change: 1 addition & 0 deletions cli/command/container/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ func TestRunPullTermination(t *testing.T) {
select {
case cmdErr := <-cmdErrC:
assert.Equal(t, cmdErr, cli.StatusError{
Cause: context.Canceled,
StatusCode: 125,
Status: "docker: context canceled\n\nRun 'docker run --help' for more information",
})
Expand Down
14 changes: 11 additions & 3 deletions cli/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

// StatusError reports an unsuccessful exit by a command.
type StatusError struct {
Cause error
Status string
StatusCode int
}
Expand All @@ -14,8 +15,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 == "" {
return "exit status " + strconv.Itoa(e.StatusCode)
if e.Status != "" {
return e.Status
}
return e.Status
if e.Cause != nil {
return e.Cause.Error()
}
return "exit status " + strconv.Itoa(e.StatusCode)
}

func (e StatusError) Unwrap() error {
return e.Cause
}
2 changes: 1 addition & 1 deletion cmd/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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()
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

What else could cancel it though?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

This improves clarity by reducing unnecessary negations.

_, _ = fmt.Fprintln(os.Stderr, err)
os.Exit(getExitCode(err))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

For example:

$ sleep 100
^C
$ echo $?
130

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

Copy link
Collaborator Author

@laurazard laurazard Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

$ 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that's also a fair point!

Copy link
Member

@thaJeztah thaJeztah Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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"

}
Expand Down
Loading