cmd/docker: add cause to user-terminated context.Context
#5760
Conversation
This patch adds a "cause" to the `context.Context` error when the user terminates the process through SIGINT/SIGTERM. This allows us to distinguish the cause of the `context.Context` cancellation. In future we would also be able to improve the UX of printed errors based on the underlying cause. Signed-off-by: Alano Terblanche <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5760 +/- ##
==========================================
- Coverage 59.47% 59.43% -0.04%
==========================================
Files 346 347 +1
Lines 29367 29422 +55
==========================================
+ Hits 17465 17488 +23
- Misses 10929 10962 +33
+ Partials 973 972 -1 |
| if err != nil && !errdefs.IsCancelled(err) { | ||
| ctx := context.Background() | ||
| err := dockerMain(ctx) | ||
| if err != nil && !errdefs.IsCancelled(err) && !errors.Is(err, errCtxUserTerminated) { |
There was a problem hiding this comment.
errors.Is(err, errCtxUserTerminated) will never be true. CancelCauseFunc sets the cause of the context error and not the error itself.
There was a problem hiding this comment.
ctx errors are propagated up from the stack as normal errors. L35 will contain the context error making the check for errors.Is(err, errCtxUserTerminated) true.
But you are correct that in this PR it won't ever be true since we are incorrectly wrapping errors downstream. A fix for that is in this PR #5666.
| if err != nil && !errdefs.IsCancelled(err) { | ||
| ctx := context.Background() | ||
| err := dockerMain(ctx) | ||
| if err != nil && !errdefs.IsCancelled(err) && !errors.Is(err, errCtxUserTerminated) { |
There was a problem hiding this comment.
Hmm, I'm wondering. In which case would err be IsCancelled and not caused by L54?
There was a problem hiding this comment.
When we cancel a prompt it returns an IsCancelled error.
There was a problem hiding this comment.
But isn't the prompt cancellation always a direct consequence of SIGINT?
There was a problem hiding this comment.
yes, but it was wired up to replace the error from context.Cancelled to errdefs.Cancelled
https://github.com/docker/cli/blob/master/cli/command/utils.go#L114-L117
https://github.com/docker/cli/blob/master/cli/command/utils.go#L78
There was a problem hiding this comment.
So why not just:
| if err != nil && !errdefs.IsCancelled(err) && !errors.Is(err, errCtxUserTerminated) { | |
| if err != nil && !errdefs.IsCancelled(err) && !errors.Is(err, context.Canceled) { |
?
There was a problem hiding this comment.
specifying an exact (custom) error here at least allows us to not accidentally ignore all context cancellation errors and instead target only those the user cancelled
There was a problem hiding this comment.
In which case the context.Canceled error isn't caused by user interrupt?
There was a problem hiding this comment.
In this PR we try to target user interrupt errors as a custom error using context.Cause. For the Go std library (like http.Transport) it would use context.Cause which means it returns the custom error we are injecting instead of context.Canceled.
This PR might not cover all cases where a user interrupt is handled with context.Cause. But it is a start!
| signal.Stop(ch) | ||
| return | ||
| case <-ch: | ||
| cancel(errCtxUserTerminated) |
There was a problem hiding this comment.
The thing that I can't wrap my mind around is:
After this cancel, we'll have:
ctx.Err() = context.Canceled
context.Cause(ctx) = errCtxUserTerminated
With that in mind, how dockerMain ends up returning the cause, instead of the ctx.Err()? 🤔
There was a problem hiding this comment.
Yes you are right, essentially right now not all commands would return a cause, but the standard library should - like http.NewRequestWithContext.
There was a problem hiding this comment.
Go std http.Transport returns a context.Cause when context.Done()
https://github.com/golang/go/blob/608acff8479640b00c85371d91280b64f5ec9594/src/net/http/transport.go#L669
Signed-off-by: Alano Terblanche <[email protected]>
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Signed-off-by: Alano Terblanche <[email protected]>
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
8839f12 to
bdd630c
Compare
This patch adds a "cause" to the
context.Contexterror when the user terminates the process through SIGINT/SIGTERM.This allows us to distinguish the cause of the
context.Contextcancellation. In future we would also be able to improve the UX of printed errorsbased on the underlying cause.
- 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)