Add async server.start() function

[glasser]

Previously, server startup worked like this:

• new ApolloServer
  • If no gateway, calculate schema and schema derived data immediately
  • If gateway, kick off gateway.load from the end of the constructor, and if it
    async-throws, log an error once and make the server kinda broken forever
• At various spots in the framework integration code, call (but don't await)
  the protected willStart function, which is an async function that first
  waits for the gateway to load the schema if necessary and then runs
  serverWillStart plugin functions; save the Promise returned by calling this.
• At request time in the framework integration code, await that Promise.
  And also, if there's no schema, fail with an error.

Now server startup works like this:

• ApolloServer represents its state explicitly with a new ServerState
• new ApolloServer
  • If no gateway, initialize all the schema-derived state directly like
    before (though the state now lives inside ServerState)
  • If gateway, the constructor DOES NOT KICK OFF gateway.load()
• You can now call await server.start() yourself, which will first await
  gateway.load if necessary, and then await all serverWillStart calls.
• If you're using apollo-server rather than an integration, server.listen()
  will just transparently do this for you; explicit start() is just for
  integrations!
• Serverless frameworks (which now all extend ApolloServerServerlessFrameworkBase)
  also call it automatically for you in the background because their startup has to be
  synchronous; if it fails then future requests will all fail (and log) as before.
• The integration places that used to call willStart now call
  server.ensureStarting() instead which will kick off server.start in the
  background if you didn't (and log any errors thrown).
• The places that used to await promiseWillStart no longer do so; generally
  right after that code we end up calling graphqlServerOptions
• graphqlServerOptions now awaits server.ensureStarted which will start the
  server if necessary and throw if it threw.

The overall change to user experience:

• If you're using apollo-server, startup errors will cause listen to reject;
  no code changes are necessary.
• If you're using a serverless integration, the behavior will be relatively similar,
  except that the startup error will be logged on all requests instead of just
  the first one.
• If you're using an integration you are encouraged to call await server.start() yourself immediately after the constructor, which will let
  you detect startup errors.
• But if you don't do that, the server will call start itself eventually. When
  you try to execute your first GraphQL request, start will happen if it
  hasn't already. Also an integration call like server.applyMiddleware will
  initiate a background start. If startup fails, the startup error will be
  logged on every failed graphql request, not just the first time like
  happened before.
• If you have your own ApolloServer subclass that calls the protected
  willStart method, it won't work because that method is gone. Consider whether
  you can eliminate that call by just calling start, or perhaps call
  ensureStarting instead.

This is close enough to backwards-compatible to be appropriate for a v2 minor
release. We are likely to make start() required in Apollo Server 3 (other than
for apollo-server).

Also:

• Previously we used the deprecated ApolloServer.schema field to determine
  whether to install ApolloServerPluginInlineTrace, which we want to have active
  by default for federated schemas only. If you're using a gateway, this field
  isn't actually set at the time that ensurePluginInstantiation reads it.
  That's basically OK because we don't want to turn on the plugin automatically
  in the gateway, but in the interest of avoiding use of the deprecated field, I
  refactored it so that ApolloServerPluginInlineTrace is installed by default
  (ie, if you don't install your own version or install
  ApolloServerPluginInlineTraceDisabled) without checking the schema, and
  then (if it's installed automatically) it decides whether or not to be active
  by checking the schema at serverWillStart time.
• Similarly, schema reporting now throws in its serverWillStart if the schema
  is federated, instead of in ensurePluginInstantiation. (This does mean that
  if you're not using the new start() or apollo-server, that failure won't
  make your app fail as fast as if the ApolloServer constructor threw.)
• Fix some fastify tests that used a fixed listen port to not do that.
• I am doing my best to never accidentally run prettier on whole files and
  instead to very carefully select specific blocks of the file to format them
  several times per minute. Apparently I screwed up once and ran it once on
  packages/apollo-server-core/src/ApolloServer.ts. The ratio of "prettier
  changes" to "actual changes" in that file is low enough that I'd rather just
  leave the changes in this PR rather than spending time carefully reverting
  them. (It's one of the files I work on the most and being able to keep it
  prettier-clean will be helpful anyway.)
• Replace a hacky workaround for the lack of start in the op reg tests!
• Replace a use of a Barrier class I added recently in tests with the
  @josephg/resolvable npm package, which does basically the same thing.
  Use that package in new tests and in the core state machine itself.
• While running tests I found that some test files hung if run separately due to
  lack of cleanup. I ended up refactoring the cache tests to:
  • make who is responsible for calling cache.close more consistent
  • make the Redis client mocks self-contained mocks of the ioredis API instead
    of starting with an actual ioredis implementation and mocking out some
    internals
  • clean up Jest fake timers when a certain test is done
    I'm not super certain exactly which of these changes fixed the hangs but it
    does seem better this way. (Specifically I think the fake timer fix, which I
    did last, is what actually fixed it, but the other changes made it easier for
    me to reason about what was going on.) Can factor out into another PR if
    helpful.

Fixes #4921. Fixes apollographql/federation#335.

TODO:

• Go through all docs and READMEs that have 'FIXME start' and add calls to
  start.
• Actually document start() in the apollo-server reference
• Document start() in all the integrations references
• CHANGELOG
• consider whether removing the protected willStart function is OK

[glasser]

@abernix This is ready for review. I think it's basically code complete. I haven't updated docs fully yet though I've tagged a bunch of places in the docs and READMEs to look at.

I do want to make sure that the various integrations are actually compatible with an async start (ie, that it's OK in something like Lambda to not assign the handler synchronously). If that doesn't work I may have to make some slight changes to those integrations and make the instructions a bit more complex.

[glasser]

I'm pretty sure that at least Lambda (and probably all of the "serverless" integrations) will not work with an API that requires you to do anything async before calling createHandler. I think the fix here is just to say that you don't call start() on serverless frameworks and rely intentionally on the ensureStarting calls in createHandler instead of just leaving them as a back-compat fallback. I think for a serverless platform it's reasonable to not really differentiate between "server failed to start up" and "request failed"? This PR will make it so that the startup failure is logged on every failure rather than just the first one for a process, which still seems reasonable. I will make this improvement tomorrow, though I think it's still worth reviewing now @abernix.

[glasser]

@abernix OK, I think I've resolved the serverless story. Serverless integrations now work pretty much like they used to: their constructors now call ensureStarting() at the end to kick off a background start() (with a slightly different message logged if this fails than if the ensureStarting() called by, say, apollo-server-express fails). I can't find any docs for any of the three frameworks that say "if your handler is totally broken call this function to kill the execution environment" so I'm just letting them have the old behavior of always failing GraphQL requests until the environment is gone (though at least it will log the startup error on every request now instead of just the first).

[StephenBarlow]

"you can should insert"

[abernix]

I left a few comments within. I have some resolvable concerns about the "readiness" (in terms of copy-pastability) of the textual-examples in the README.md files given that the async startApolloServer functions are not being invoked in the functions which now wrap that logic, but I can understand the approach that is taken since it would resolve a different limitation (e.g., top-level await). With top-level await being supported out of the box in Node.js 14, I suspect those examples can be changed back to a more simple usage in the not-too-distant future, presumably when Apollo Server 3.x makes the minimum supported Node.js version v14 (as I suspect it should/could!)

All in all, this is an important long-needed API and I'm glad this brings that. The documentation and some of the severity of the changes (e.g., the recommended change to the usage and the removal of the willStart method) coupled with the need (within this PR) to concern ourselves with backcompat does leave me to believe that this might have paired even better with Apollo Server 3.x but this certainly does solve problems which have manifested in one case or another for some users for some time now, so 👍 .

[abernix]

This example could benefit from calling startApolloServer, otherwise it may be indiscernible to someone copying and pasting this example as to why the server hasn't started.

[glasser]

I mean, the problem is... how are you supposed to call it? The whole point is I don't want to encourage people to call it in a context where errors will turn into unhandledRejection. So I could show it being called from top-level await... but then I might as well just revert back to not having a function. Or I could show it being called with .catch() but that seems like it would get distracting (and it's unclear that a particular bit of .catch() from our docs is actually the best way to integrate it into your system).

While Node does now support top-level await (as of v14.8.0), it requires you to be in a special "module" mode where your file ends with .mjs, or set a flag in package.json. And it's different in TypeScript... here you have to be in module module (which should be the case for any of these examples because of the use of import) but you also have to set the target and/or module tsconfig option appropriately...

It feels like this is a big bundle of worms, compared to "here's an async function and it's your job to know how to use it". Since this mostly affects "advanced" uses (I'm not editing any calls to apollo-server's listen()) I'm a little less worried?

I mean, the fact that our apollo-server examples tell you to call listen() with only .then and not .catch is another issue, and perhaps we should go through and change those all to use top-level await with explicit guidance to use .mjs files, but that's a project for another day...

[abernix]

Same concerns as above about startApolloServer not being invoked.

[abernix]

Same concerns as above about startApolloServer not being invoked within the examples. This would yield a non-functioning server right now and we have expressly (pun certainly intended for this express integration!) agreed in the past that examples should just work. Can we just invoke it?

[abernix]

Is it necessary for this to be protected? I didn't catch this being invoked outside of this particular file and I don't quite know if we're expecting someone to subclass it.

[glasser]

It's called by apollo-server's listen().
I'll document that it's not guaranteed to be stable.

[abernix]

I could very much believe that an unknown number of third-party Apollo Server integrations or any extensions to ApolloServerBase might be relying on this. Anyway to shim it?

[glasser]

Yeah, the deal is that it's roughly equivalent to calling start(), but the places that use it should probably just call ensureStarting() instead. My first instinct today was to have it do a deprecation log, but fixing it is probably something you'll need to ask your integration to do, not something you can fix yourself, so the deprecation log might get annoying. I'm going to leave out the log and try to replicate the old kinda weird semantics where only some errors throw.

We can remove willStart in AS3 (#5050)

If you think I should add a deprecation warning we can do that though right now I'm feeling like just leaving it mostly working and removing it in AS3 is fine.

[glasser]

I'll merge this today but I'm happy to continue the conversation about sample code. My main instinct is I'd rather have sample code that requires users to be slightly creative than sample code that either will give weird error messages if they don't have top-level await enabled or that will encourage users to run async functions without handling possible rejections.

Re the AS3 question, I agree that not thinking about back-compat could be simple, but to be honest I expect we will learn interesting things about the impact of this change when we release it post-alpha-phase, and I'd like to be able to carry those learnings over to AS3 instead of only releasing this change as part of AS3.

[glasser]

I have an alpha out that resolves this issue.

First, install the alpha in your app by installing v2.22.0-alpha.0 of whatever Apollo Server packages your app directly depends on. This might look something like

npm i apollo-server-express@2.22.0-alpha.0 apollo-server-core@2.22.0-alpha.0

If you're using the apollo-server package (and calling listen()), that should be enough.

Otherwise, if you're using (say) apollo-server-express, you'll want to insert a call to await server.start() between server = new ApolloServer and server.applyMiddleware. This does mean you'll want to be setting up your server in a context where you can await (an async function, the top level in a .mjs file, etc). This call is technically optional but you don't get the improved error handling if you don't do it!

If you're using a serverless package like apollo-server-lambda, this doesn't fully improve the situation, though I'd still be interested in hearing about how it works for you.

Please provide feedback at #5051.

More details at https://www.apollographql.com/docs/apollo-server/api/apollo-server/#start (though oops, the docs shouldn't be up until the API is in a non-alpha version, so if this doesn't get released pretty soon I might roll that back).

[glasser]

This is released as Apollo Server v2.22.0!