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

Wrap main and remove os.Exit calls so all deferred functions will execute #1693

Merged
merged 18 commits into from
Apr 30, 2024

Conversation

RebeccaMahany
Copy link
Contributor

@RebeccaMahany RebeccaMahany commented Apr 25, 2024

Relates to #1692

When we call os.Exit, deferred functions will not execute, which means e.g. the event log writer on windows won't get closed. For a more graceful shutdown, we wrap main as runMain and disallow os.Exit calls outside of the new main.

cmd/launcher/main.go Outdated Show resolved Hide resolved
cmd/launcher/svc.go Outdated Show resolved Hide resolved
cmd/launcher/svc.go Outdated Show resolved Hide resolved
James-Pickett
James-Pickett previously approved these changes Apr 25, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I think it's the right shape, but I wonder what we can do with these errors.

main doesn't have a logger, and stdout may not go anywhere. I wonder about a roundabout thing where runMain returns a slogger, and and error, and if the slogger isn't nil it can log the error... Or we can make sure all the error returns log first.

cmd/launcher/main.go Outdated Show resolved Hide resolved
cmd/launcher/main.go Outdated Show resolved Hide resolved
directionless
directionless previously approved these changes Apr 29, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I didn't test this, but it looks okay

.golangci.yml Show resolved Hide resolved
cmd/launcher/main.go Show resolved Hide resolved
cmd/launcher/svc_windows.go Show resolved Hide resolved
@RebeccaMahany RebeccaMahany marked this pull request as ready for review April 29, 2024 19:52
directionless
directionless previously approved these changes Apr 30, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I think I'm game. I don't think I was that confident when I wrote service manager stuff, so this feels like a reasonable upgrade.

cmd/launcher/main.go Show resolved Hide resolved
cmd/launcher/svc_windows.go Show resolved Hide resolved
@RebeccaMahany RebeccaMahany added this pull request to the merge queue Apr 30, 2024
Merged via the queue into kolide:main with commit 7e4497e Apr 30, 2024
29 checks passed
@RebeccaMahany RebeccaMahany deleted the becca/no-os-exit branch April 30, 2024 13:44
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.

4 participants