cmd/docker: add cause to user-terminated context.Context#5760
Merged
Benehiko merged 1 commit intodocker:masterfrom Feb 5, 2025
Merged
cmd/docker: add cause to user-terminated context.Context#5760Benehiko merged 1 commit intodocker:masterfrom
context.Context#5760Benehiko merged 1 commit intodocker:masterfrom
Conversation
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 29431 +64
==========================================
+ Hits 17465 17492 +27
- Misses 10929 10965 +36
- Partials 973 974 +1 |
vvoland
reviewed
Jan 21, 2025
vvoland
reviewed
Jan 24, 2025
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
8839f12 to
bdd630c
Compare
ndeloof
approved these changes
Jan 29, 2025
fayazkhan121
approved these changes
Jan 29, 2025
fayazkhan121
left a comment
fayazkhan121
left a comment
There was a problem hiding this comment.
The test looks solid. TestUserTerminatedError test case properly introduces notifyContext and handles the termination signals as expected.
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
1cad6c0 to
da5701e
Compare
Member
Author
|
Please don't merge yet, this is incorrect. We need to have status 130 print when a container exits with that code. |
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
da5701e to
bdd630c
Compare
fayazkhan121
reviewed
Feb 3, 2025
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
577e0d2 to
4e7cf34
Compare
thaJeztah
dismissed
fayazkhan121’s stale review
Spam / AI account (blocked now)
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
4e7cf34 to
8e41a36
Compare
Collaborator
|
LGTM overall, but we probably still need to adjust the output, see: BeforeAfterIMO, it shouldn't print anything at all, so it should look like: |
Member
Author
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
8e41a36 to
9f41558
Compare
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]> cmd/docker: fix possible race between ctx channel and signal channel Signed-off-by: Alano Terblanche <[email protected]> test: notifyContext Signed-off-by: Alano Terblanche <[email protected]> cmd/docker: print status on SIGTERM and not SIGINT Signed-off-by: Alano Terblanche <[email protected]>
Benehiko
force-pushed
the
user-terminated-ctx-err
branch
from
9f41558 to
c51be77
Compare
laurazard
approved these changes
Feb 3, 2025
Comment on lines
+31
to
+37
| type errCtxSignalTerminated struct { | ||
| signal os.Signal | ||
| } | ||
|
|
||
| func (e errCtxSignalTerminated) Error() string { | ||
| return "" | ||
| } |
Member
There was a problem hiding this comment.
Just a small nit/suggestion, not necessary.
You could do:
Suggested change
| type errCtxSignalTerminated struct { | |
| signal os.Signal | |
| } | |
| func (e errCtxSignalTerminated) Error() string { | |
| return "" | |
| } | |
| type errCtxSignalTerminated struct { | |
| signal os.Signal | |
| } | |
| func (e *errCtxSignalTerminated) Error() string { | |
| return "" | |
| } | |
| func (e *errCtxSignalTerminated) exitCode() int { | |
| if e == nil { | |
| return 1 | |
| } | |
| s, ok := e.signal.(syscall.Signal) | |
| if !ok { | |
| return 1 | |
| } | |
| return 128 + int(s) | |
| } |
Then errCtxSignalTerminated implements error (instead of *errCtxSignalTerminated implementing error.
Member
Author
There was a problem hiding this comment.
The suggestion looks alright to me, although we only use the exit code inside the getExitCode function and this error type is only used inside cmd/docker/docker.go. At least to me, it just seems like extra refactoring with little benefit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)