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.

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

Only gracefully exit rpcchainvm server after Shutdown #1988

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

This gives us the following nice properties:

  1. This solution doesn't require user intervention (systemd will just start working correctly for newer plugin versions)
  2. We do not ungracefully kill the subprocess (as it is able to catch the signal we send and gracefully exit with code 0)
  3. We still use a signal designed for immediately terminating a process to terminate the subprocess

How this works

  1. Upon startup register a signal handler to SIGINT and SIGTERM on the plugin side to avoid systemd from unexpectedly killing the subprocess.
  2. During the shutdown flow avalanchego will call Shutdown over the gRPC.
  3. During the handling of the Shutdown call - update the signal handler to now honor SIGTERM signals.
  4. After Shutdown has been called send the SIGTERM call as we did previously.

How this was tested

CI

@patrick-ogrady
Copy link
Contributor

patrick-ogrady commented Sep 8, 2023

I think we may be able to unify the stopper logic as well:

//go:build !linux
// +build !linux
package subprocess
import (
"context"
"os/exec"
"go.uber.org/zap"
"github.com/ava-labs/avalanchego/utils/logging"
)
func NewCmd(path string, args ...string) *exec.Cmd {
return exec.Command(path, args...)
}
func stop(_ context.Context, log logging.Logger, cmd *exec.Cmd) {
err := cmd.Process.Kill()
if err == nil {
log.Debug("subprocess was killed")
} else {
log.Error("subprocess was killed",
zap.Error(err),
)
}
}

Both SIGTERM/SIGINT are supported on Windows/Darwin:

@hexfusion
Copy link
Contributor

hexfusion commented Sep 8, 2023

I think they exist but are not implemented as expected in Windows more details on Interrupt[1] .

os/signal docs[2] are a little hard to grok.

On Windows a ^C (Control-C) or ^BREAK (Control-Break) normally cause the program to exit. If Notify is called for os.Interrupt, ^C or ^BREAK will cause os.Interrupt to be sent on the channel, and the program will not exit. If Reset is called, or Stop is called on all channels passed to Notify, then the default behavior will be restored.

I believe that is a direct reference to "console process group" which would not be our intended use.

[1] golang/go#46345 (comment)
[2] https://pkg.go.dev/os/signal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants