apollo-server-core: register signal handlers later and not on serverless #5639
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
`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
force-pushed
the
glasser/register-handlers-later
branch
from
ee77696
to
ef6447b
Compare
glasser
added a commit
that referenced
this pull request
Aug 20, 2021
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. - 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.
glasser
added a commit
that referenced
this pull request
Aug 20, 2021
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. - 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.
glasser
added a commit
that referenced
this pull request
Aug 23, 2021
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. - 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.
glasser
added a commit
that referenced
this pull request
Aug 23, 2021
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. - 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.
glasser
added a commit
that referenced
this pull request
Aug 23, 2021
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.
glasser
added a commit
that referenced
this pull request
Aug 23, 2021
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.
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.
stop()is not designed to work properly ifstart()has notpreviously succeeded (there's an XXX comment about this), and #5635 is
going to make early
stop()calls into a hard error. This changeensures 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
startorlistenyourself), 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