fix: force cli to exit after third sigint/sigterm #5171
Conversation
…lation Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5171 +/- ##
==========================================
- Coverage 61.76% 61.72% -0.05%
==========================================
Files 297 294 -3
Lines 20768 20772 +4
==========================================
- Hits 12828 12822 -6
- Misses 7024 7034 +10
Partials 916 916 |
krissetto
left a comment
There was a problem hiding this comment.
maybe we should also add a basic test case for this
laurazard
left a comment
There was a problem hiding this comment.
I had another idea, let me know what you think!
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
| <-sig | ||
| } | ||
| _, _ = fmt.Fprint(w, "\ngot 3 SIGTERM/SIGINTs, forcefully exiting\n") | ||
| os.Exit(1) |
There was a problem hiding this comment.
Totally non blocking, just food for thought..are we sure we want to exit with status 1 when the user decides to forcefully exit?
It could make sense to start differentiating expected status codes from totally unexpected failures, in order to be able to distinguish them from a tracing/metrics perspective as well.
There was a problem hiding this comment.
I think forcefully exiting the CLI like this should be considered a problem. It means that the process the user cancelled did not cancel the first time through context.Done().
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
68e3f29 to
faf7647
Compare
- What I did
Added a global signal handler for cases where context cancellation is not respected. This will force the CLI to exit after 3 attempts of sigint/sigterm.
- How I did it
I register a go routine to catch further signals upon running a docker command. Docker plugin handling has its own signal handling and is excluded from this signal handler.
- How to verify it
Try with
docker loginsince it's currently not respecting context cancellation.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)