Skip to content

fix: force cli to exit after third sigint/sigterm #5171

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

Merged
vvoland merged 3 commits into from
Jun 20, 2024
Merged

fix: force cli to exit after third sigint/sigterm #5171

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4bfd8c
feat: add a global sigint/sigterm handler as a fallback to ctx cancel…
Benehiko Jun 18, 2024
1322f58
test: cli force exit signal handler
Benehiko Jun 20, 2024
faf7647
refactor: remove test
Benehiko Jun 20, 2024
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
24 changes: 24 additions & 0 deletions cmd/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"context"
"fmt"
"io"
"os"
"os/exec"
"os/signal"
Expand Down Expand Up @@ -325,6 +326,25 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command
return nil
}

// forceExitAfter3TerminationSignals waits for the first termination signal
// to be caught and the context to be marked as done, then registers a new
// signal handler for subsequent signals. It forces the process to exit
// after 3 SIGTERM/SIGINT signals.
func forceExitAfter3TerminationSignals(ctx context.Context, w io.Writer) {
// wait for the first signal to be caught and the context to be marked as done
<-ctx.Done()
// register a new signal handler for subsequent signals
sig := make(chan os.Signal, 2)
signal.Notify(sig, platformsignals.TerminationSignals...)

// once we have received a total of 3 signals we force exit the cli
for i := 0; i < 2; i++ {
<-sig
}
_, _ = fmt.Fprint(w, "\ngot 3 SIGTERM/SIGINTs, forcefully exiting\n")
os.Exit(1)
Copy link
Contributor

@krissetto krissetto Jun 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Benehiko Benehiko Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

}

//nolint:gocyclo
func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
tcmd := newDockerCommand(dockerCli)
Expand Down Expand Up @@ -384,6 +404,10 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
}
}

// This is a fallback for the case where the command does not exit
// based on context cancellation.
go forceExitAfter3TerminationSignals(ctx, dockerCli.Err())

// We've parsed global args already, so reset args to those
// which remain.
cmd.SetArgs(args)
Expand Down