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

fix Windows Service timeout during high CPU (eg. post Windows Update) #1047

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

jammiemil
Copy link
Contributor

This PR attempts to resolve a long standing issue with the windows service timing out when CPU is constrained on the host machine.
The underlying cause of this is due to how late the service initialization happens in the code due to it existing in main() which is the last part of code to be run during startup as per:

image

What we attempt to do here is call the windows service as early as possible by placing it in a separate package's init() at the top of the main packages import list causing it to occur as early in the process as possible
Originally I had also moved the initiation of logging into this same package however i ran into issues with double logging in event viewer so ill revisit this at a later date.
The benefit of having the logging called as early as possible is that you can properly debug issues such as this as until logging is initiated as part of main() in the main package none of the log commands can actually execute

Signed-off-by: Jamie Milton <jammiemil@hotmail.com>
@jammiemil
Copy link
Contributor Author

As it stands this code will initiate a service super fast but i have subsequently broken the 'stop' of a service and am struggling to work out how best to make this work. The cause of this is the channel used to inform on service stop requests is not acted on in main(), only in the new initiate package, and im getting out of my depth :)

Signed-off-by: Jamie Milton <jammiemil@hotmail.com>
@jammiemil jammiemil marked this pull request as ready for review August 23, 2022 15:48
@jammiemil jammiemil requested a review from a team as a code owner August 23, 2022 15:48
Signed-off-by: Jamie Milton <jammiemil@hotmail.com>
Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

This is looking nice! The change is mostly moving code out of the main exporter package, so the only improvement I can think of is additional documentation in the form of some comments.

Thanks for putting in the effort to resolve this issue 👍

exporter.go Outdated Show resolved Hide resolved
initiate/initiate.go Show resolved Hide resolved
Signed-off-by: Jamie Milton <jammiemil@hotmail.com>
Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

Looking good 👍
I'm happy to merge if you're comfortable with your testing.

@jammiemil
Copy link
Contributor Author

I've tested this a fair bit by replacing the binary in a VM and the service certainly starts much quicker under load + service stop/start/restart functionality does not appear to be hampered.

The only thing i haven't tested i guess is installing it from scratch using the msi since i wasn't able to build this but i see no reason why that would be affected since that code hasn't been touched.

@breed808
Copy link
Contributor

I agree, the MSI installation shouldn't be affected.

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.

2 participants