This repository has been archived by the owner on Mar 22, 2019. It is now read-only.
Add option for 'StartupHook' #35
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kanatohodets
added a commit
to kanatohodets/carbonsearch
that referenced
this pull request
Feb 1, 2017
PR has been submitted (facebookarchive/grace#35) but we'd like to cut a release without blocking on the PR discussion. Note: the scope of the fork (and the lack of package renames in the fork) means that _both_ the fork and upstream are now deps.
kanatohodets
added a commit
to kanatohodets/carbonsearch
that referenced
this pull request
Feb 1, 2017
PR has been submitted (facebookarchive/grace#35) but we'd like to cut a release without blocking on the PR discussion. Note: the scope of the fork (and the lack of package renames in the fork) means that _both_ the fork and upstream are now deps.
daaku
suggested changes
Feb 16, 2017
There was a problem hiding this comment.
Looks pretty good and I understand the use case. The name StartupHook was a bit confusing to me at lease. What do you think about calling it PreStartProcess? I like it better because it gets called right before we call StartProcess. Admittedly no name is going to make it super clear what this is.
gracehttp/http_test.go
Outdated
| @@ -19,6 +20,10 @@ import ( | |||
| "github.com/facebookgo/freeport" | |||
| ) | |||
|
|
|||
| const ( | |||
| TEST_STARTUP_HOOK = iota | |||
gracehttp/testbin_test.go
Outdated
| @@ -16,6 +16,8 @@ import ( | |||
| "github.com/facebookgo/grace/gracehttp" | |||
| ) | |||
|
|
|||
| const STARTUP_HOOK_ENV = "GRACEHTTP_STARTUP_HOOK" | |||
This adds support for options to be added to 'Serve' and the app struct. Options are implemented following the 'functional options' pattern (https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis and https://commandcenter.blogspot.co.uk/2014/01/self-referential-functions-and-design.html). Future options can be added by creating an exported func that returns a closure modifying the app struct, like the following: func HaltAndCatchFire(literallyCatchFire bool) option { return func(a *app) { a.haltAndCatchFire = literallyCatchFire } } then in user code: gracehttp.ServeWithOptions( []*http.Server{ &myServer }, gracehttp.HaltAndCatchFire(true), )
This option attaches a callback to the application. This callback is triggered
directly before the new process is started during a graceful restart. This
allows the old process to release its hold on any resources that the new
process will need.
For example:
gracehttp.ServeWithOptions(
[]*http.Server{ &myServer },
gracehttp.StartupHook(func () error {
// release port that new process will need to start up successfully
return nil
}
)
This better indicates the timing of the callback by using terms already present in the codebase. As part of the rename, the related constants in the tests were fixed to follow the naming convention.
|
Thanks for the notes! Renamed to 'PreStartProcess', fixed the test constants to follow naming convention, and rebased to get the log change. |
|
Thanks for the feature! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hiya!
First: thanks for grace, it is the bomb.
At $work we bumped into a situation where graceful restart was impeded by a resource the old process needed to release in order for the new one to start up successfully. In our specific case, there was an auxiliary HTTP server running on a different port that we needed to shut off before booting up the successor process: otherwise the port would still be in use. We needed to ensure the successor could start that auxiliary HTTP server because the application is an in-memory index, and it needs to receive data on that auxiliary server in order to warm up and be ready to serve queries on the primary HTTP server.
This PR introduces a second exported func,
ServeWithOptions, which takes a slice of servers and varargs functional options (following the pattern from here). The only option implemented at present is support for aStartupHook, which takes afunc() errorand calls that after receivingSIGUSR2but before starting the successor. This provides an opportunity to release holds on resources without the potential race of having anotherSIGUSR2handler.This library has been stable for quite a while, so I totally understand if you'd prefer not to accept a change of this magnitude for a use-case that is a little on the specific side. Also more than glad to do any cleaning if you think it might be suitable with some polish; in particular it took me a little bit to get my head around the tests.
Thanks!