Generalize apollo-server graceful shutdown to all integrations#5635
Merged
Generalize apollo-server graceful shutdown to all integrations#5635
Conversation
IvanGoncharov
approved these changes
Aug 20, 2021
Contributor
IvanGoncharov
left a comment
There was a problem hiding this comment.
@glasser Code level changes look good and the idea to drain the server as a separate phase also makes a lot of sense.
glasser
added a commit
that referenced
this pull request
Aug 20, 2021
`stop()` is not designed to work properly if `start()` has not previously succeeded (there's an XXX comment about this), and #5635 is going to make early `stop()` calls into a hard error. This change ensures that the signal handler won't cause that error to show up. Also, serverless integrations don't use the same sort of process-based shutdown as other environments (and you don't call `start` or `listen` yourself), so registering these signal handlers isn't a great default. (They start "listening" before the ApolloServer is started, so it would be weird if after this change the signal handling depended on whether or not a request had been processed or not.) Make stopOnTerminationSignals default to false for serverless integrations.
glasser
added a commit
that referenced
this pull request
Aug 20, 2021
`stop()` is not designed to work properly if `start()` has not previously succeeded (there's an XXX comment about this), and #5635 is going to make early `stop()` calls into a hard error. This change ensures that the signal handler won't cause that error to show up. Also, serverless integrations don't use the same sort of process-based shutdown as other environments (and you don't call `start` or `listen` yourself), so registering these signal handlers isn't a great default. (They start "listening" before the ApolloServer is started, so it would be weird if after this change the signal handling depended on whether or not a request had been processed or not.) Make stopOnTerminationSignals default to false for serverless integrations.
glasser
added a commit
that referenced
this pull request
Aug 20, 2021
…ess (#5639) `stop()` is not designed to work properly if `start()` has not previously succeeded (there's an XXX comment about this), and #5635 is going to make early `stop()` calls into a hard error. This change ensures that the signal handler won't cause that error to show up. Also, serverless integrations don't use the same sort of process-based shutdown as other environments (and you don't call `start` or `listen` yourself), so registering these signal handlers isn't a great default. (They start "listening" before the ApolloServer is started, so it would be weird if after this change the signal handling depended on whether or not a request had been processed or not.) Make stopOnTerminationSignals default to false for serverless integrations.
d47459b to
37e32c6
Compare
2c375f1 to
51a6b19
Compare
Previously, the batteries-included `apollo-server` package had a special override of `stop()` which drains the HTTP server before letting the actual Apollo Server `stop()` machinery begin. This meant that `apollo-server` followed this nice shutdown lifecycle: - Stop listening for new connections - Close all idle connections and start closing connections as they go idle - Wait a grace period for all connections to close and force-close any remaining ones - Transition ApolloServer to the stopping state, where no operations will run - Run stop hooks (eg send final usage report) This was great... but only `apollo-server` worked this way, because only `apollo-server` has full knowledge and control over its HTTP server. This PR adds a server draining step to the ApolloServer lifecycle and plugin interface, and provides a built-in plugin which drains a Node `http.Server` using the logic of the first three steps above. `apollo-server`'s behavior is now just to automatically install the plugin. Specifically: - Add a new 'phase' called `draining` that fits between `started` and `stopping`. Like `started`, operations can still execute during `draining`. Like `stopping`, any concurrent call to `stop()` will just block until the first `stop()` call finishes rather than starting a second shutdown process. - Add a new `drainServer` plugin hook (on the object returned by `serverWillStart`). Invoke all `drainServer` hooks in parallel during the `draining` phase. - Make calling `stop()` when `start()` has not yet completed successfully into an error. That behavior was previously undefined. Note that as of #5639, the automatic `stop()` call from signal handlers can't happen before `start()` succeeds. - Add `ApolloServerPluginDrainHttpServer` to `apollo-server-core`. This plugin implements `drainServer` using the `Stopper` class that was previously in the `apollo-server` package. The default grace period is 10 seconds. - Clean up integration tests to just use `stop()` with the plugin instead of separately stopping the HTTP server. Note that for Fastify specifically we also call `app.close` although there is some weirdness here around both `app.close` and our Stopper closing the same server. A comment describes the weirdness; perhaps Fastify experts can improve this later. - The Hapi web framework has built in logic that is similar to our Stopper, so `apollo-server-hapi` exports `ApolloServerPluginStopHapiServer` which should be used instead of the other plugin with Hapi. - Remove some examples from READMEs and point to examples in the docs instead. Keeping both up to date is extra work. - Fix some test issues (eg, have FakeTimers only mock out Date.now instead of setImmediate, drop an erroneous `const` which made an `app` not get cleaned up, etc). Fixes #5074.
51a6b19 to
a59574e
Compare
Member
Author
|
I'd like to move forward with releasing this, but it would be great to have a second round of post-merge eyes on this from @IvanGoncharov. And @StephenBarlow , you may have thoughts on the docs! |
This file contains hidden or 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
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.
Previously, the batteries-included
apollo-serverpackage had a specialoverride of
stop()which drains the HTTP server before letting theactual Apollo Server
stop()machinery begin. This meant thatapollo-serverfollowed this nice shutdown lifecycle:idle
remaining ones
will run
This was great... but only
apollo-serverworked this way, because onlyapollo-serverhas full knowledge and control over its HTTP server.This PR adds a server draining step to the ApolloServer lifecycle and
plugin interface, and provides a built-in plugin which drains a Node
http.Serverusing the logic of the first three steps above.apollo-server's behavior is now just to automatically install theplugin.
Specifically:
drainingthat fits betweenstartedandstopping. Likestarted, operations can still execute duringdraining. Likestopping, any concurrent call tostop()will justblock until the first
stop()call finishes rather than starting asecond shutdown process.
drainServerplugin hook (on the object returned byserverWillStart). Invoke alldrainServerhooks in parallel duringthe
drainingphase.stop()whenstart()has not yet completedsuccessfully into an error. That behavior was previously undefined.
Note that as of apollo-server-core: register signal handlers later and not on serverless #5639, the automatic
stop()call from signalhandlers can't happen before
start()succeeds.ApolloServerPluginDrainHttpServertoapollo-server-core.This plugin implements
drainServerusing theStopperclassthat was previously in the
apollo-serverpackage. The defaultgrace period is 10 seconds.
stop()with the plugininstead of separately stopping the HTTP server. Note that for Fastify
specifically we also call
app.closealthough there is some weirdnesshere around both
app.closeand our Stopper closing the same server.A comment describes the weirdness; perhaps Fastify experts can improve
this later.
Stopper, so
apollo-server-hapiexportsApolloServerPluginStopHapiServerwhich should be used instead of theother plugin with Hapi.
instead. Keeping both up to date is extra work.
instead of setImmediate, drop an erroneous
constwhich made anappnot get cleaned up, etc).
Fixes #5074.