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

Restart server on failure when running Windows app #3985

Merged
merged 5 commits into from Apr 29, 2024

Conversation

jmorganca
Copy link
Member

No description provided.

@jmorganca jmorganca requested a review from dhiltgen April 28, 2024 02:49
@@ -143,15 +147,12 @@ func SpawnServer(ctx context.Context, command string) (chan int, error) {
default:
crashCount++
slog.Warn(fmt.Sprintf("server crash %d - exit code %d - respawning", crashCount, code))
time.Sleep(500 * time.Millisecond)
if err := cmd.Start(); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

Start() can only be called once for each exec.Cmd instance and so we need to create a new one on every restart

}
}

func start(ctx context.Context, command string) (*exec.Cmd, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

start factors out the setup of the exec.Cmd

Copy link
Collaborator

@dhiltgen dhiltgen left a comment

Choose a reason for hiding this comment

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

One glitch, but otherwise looks good.

slog.Info("starting server...")
cmd, err := start(ctx, command)
if err != nil {
slog.Error(fmt.Sprintf("failed to start server %s", err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should continue the for loop so we don't crash on a nil pointer with cmd.Wait() below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah great catch. Thank you!

@jmorganca jmorganca merged commit 95ead8f into main Apr 29, 2024
12 checks passed
@jmorganca jmorganca deleted the jmorganca/restart-server branch April 29, 2024 14:07
@gamunu gamunu mentioned this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants