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

Add support for graceful reload #69

Merged
merged 5 commits into from
Jun 6, 2019
Merged

Add support for graceful reload #69

merged 5 commits into from
Jun 6, 2019

Conversation

nikmolnar
Copy link
Contributor

@nikmolnar nikmolnar commented May 10, 2019

This PR adds support for gracefully reloading the server configuration (without dropping any in-progress connections or refusing any new ones) upon receiving a HUP signal.

$ kill -HUP <main pid>

(the main pid will always be the smaller one)

The strategy is to fork a child process on start, rather than handling requests in the main process. The main process binds a listener to the desired port and passes that listener to the child process so that it can accept new requests.

When a reload is requested, the parent process creates a replacement child process, passing it the same listener. It send the original child process a signal to gracefully shutdown, after fulfilling pending connections. It also implements a timer, ensuring the process is stopped eventually in the event of a hang. The parent is now ready for another reload signal.

As far as testing, I've continually reloaded the server, while requesting tiles via the mbtileserver service map page using a throttled connection, and have not seen any tiles drop.

@nikmolnar
Copy link
Contributor Author

(the main pid will always be the smaller one)

Well, unless the main process gets the last pid before the OS cycles back to the beginning. In which case, the main pid will be much larger.

@coveralls
Copy link

coveralls commented May 10, 2019

Coverage Status

Coverage decreased (-3.0%) to 6.758% when pulling ef19902 on reload-v2 into d5769a0 on master.

Copy link
Collaborator

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

@nikmolnar nicely done! Appears to work as expected.

Please add usage instructions to the README.


server.TLSConfig.GetCertificate = e.AutoTLSManager.GetCertificate
server.TLSConfig.NextProtos = append(server.TLSConfig.NextProtos, acme.ALPNProto)
if !e.DisableHTTP2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no server.StartAutoTLS to replace e.StartAutoTLS, so I stole the implementation of e.StartAutoTLS and just in-lined it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is part of that implementation.

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.

3 participants