Skip to content

Commit

Permalink
Add async server.start() function
Browse files Browse the repository at this point in the history
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!
- 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 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 before 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. This involves verifying that you can actually do top-level await in
  the contexts that matter. (eg if it turns out that you really can't call await
  before you assign a handler in Lambda, that's interesting and may require some
  other changes to this PR!)
- [ ] 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
  • Loading branch information
glasser committed Mar 22, 2021
1 parent 91d779a commit e552e3b
Show file tree
Hide file tree
Showing 46 changed files with 816 additions and 506 deletions.
1 change: 1 addition & 0 deletions docs/source/api/apollo-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,7 @@ const server = new ApolloServer({
typeDefs,
resolvers,
});
// FIXME add start here

const app = express();

Expand Down
2 changes: 1 addition & 1 deletion docs/source/data/subscriptions.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ const express = require('express');
const PORT = 4000;
const app = express();
const server = new ApolloServer({ typeDefs, resolvers });

// FIXME start
server.applyMiddleware({app})

const httpServer = http.createServer(app);
Expand Down
4 changes: 2 additions & 2 deletions docs/source/deployment/azure-functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ npm init -y
npm install apollo-server-azure-functions graphql
```

Copy the code below and paste at you **index.js** file.
Copy the code below and paste in your **index.js** file.

```javascript
const { ApolloServer, gql } = require('apollo-server-azure-functions');
Expand All @@ -84,7 +84,7 @@ const resolvers = {
};

const server = new ApolloServer({ typeDefs, resolvers });

// FIXME start
exports.graphqlHandler = server.createHandler();
```

Expand Down
4 changes: 4 additions & 0 deletions docs/source/deployment/lambda.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const resolvers = {
};

const server = new ApolloServer({ typeDefs, resolvers });
// FIXME start

exports.graphqlHandler = server.createHandler();
```
Expand Down Expand Up @@ -164,6 +165,7 @@ const server = new ApolloServer({
context,
}),
});
// FIXME start

exports.graphqlHandler = server.createHandler();
```
Expand All @@ -190,6 +192,7 @@ const resolvers = {
};

const server = new ApolloServer({ typeDefs, resolvers });
// FIXME start

exports.graphqlHandler = server.createHandler({
cors: {
Expand Down Expand Up @@ -219,6 +222,7 @@ const resolvers = {
};

const server = new ApolloServer({ typeDefs, resolvers });
// FIXME start

exports.graphqlHandler = server.createHandler({
cors: {
Expand Down
3 changes: 2 additions & 1 deletion docs/source/deployment/netlify.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,14 @@ const server = new ApolloServer({
typeDefs,
resolvers
});
// FIXME start

exports.handler = server.createHandler();
```

Now, make sure you've run `NODE_ENV=development npm run start:lambda`, and navigate to `localhost:9000/graphql` in your browser. You should see GraphQL Playground, where you can run queries against your API!

*Note - The GraphQL Playground will only run if your `NODE_ENV` is set to `development`. If you don't pass this, or your `NODE_ENV` is set to `production`, you will not see the GraphQL Playground.*
*Note - The GraphQL Playground will only run if your `NODE_ENV` is set to `development`. If you don't pass this, or your `NODE_ENV` is set to `production`, you will not see the GraphQL Playground.*

![Local GraphQL Server](../images/graphql.png)

Expand Down
1 change: 1 addition & 0 deletions docs/source/integrations/middleware.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const server = new ApolloServer({
typeDefs,
resolvers,
});
// FIXME start

server.applyMiddleware({ app });

Expand Down
3 changes: 2 additions & 1 deletion docs/source/security/terminating-ssl.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ const environment = process.env.NODE_ENV || 'production'
const config = configurations[environment]

const apollo = new ApolloServer({ typeDefs, resolvers })
// FIXME start

const app = express()
apollo.applyMiddleware({ app })

// Create the HTTPS or HTTP server, per configuration
var server
if (config.ssl) {
// Assumes certificates are in a .ssl folder off of the package root. Make sure
// Assumes certificates are in a .ssl folder off of the package root. Make sure
// these files are secured.
server = https.createServer(
{
Expand Down
6 changes: 6 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
},
"devDependencies": {
"@apollographql/graphql-upload-8-fork": "8.1.3",
"@josephg/resolvable": "^1.0.0",
"@types/async-retry": "1.4.2",
"@types/aws-lambda": "8.10.66",
"@types/body-parser": "1.19.0",
Expand Down
3 changes: 3 additions & 0 deletions packages/apollo-server-azure-functions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const resolvers = {
};

const server = new ApolloServer({ typeDefs, resolvers });
// FIXME start

module.exports = server.createHandler();
```
Expand Down Expand Up @@ -86,6 +87,7 @@ const server = new ApolloServer({
typeDefs,
resolvers,
});
// FIXME start

module.exports = server.createHandler({
cors: {
Expand Down Expand Up @@ -118,6 +120,7 @@ const server = new ApolloServer({
typeDefs,
resolvers,
});
// FIXME start

module.exports = server.createHandler({
cors: {
Expand Down
6 changes: 0 additions & 6 deletions packages/apollo-server-azure-functions/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,6 @@ export class ApolloServer extends ApolloServerBase {
}

public createHandler({ cors }: CreateHandlerOptions = { cors: undefined }) {
// We will kick off the `willStart` event once for the server, and then
// await it before processing any requests by incorporating its `await` into
// the GraphQLServerOptions function which is called before each request.
const promiseWillStart = this.willStart();

const corsHeaders: HttpResponse['headers'] = {};

if (cors) {
Expand Down Expand Up @@ -144,7 +139,6 @@ export class ApolloServer extends ApolloServerBase {
);
};
graphqlAzureFunction(async () => {
await promiseWillStart;
return this.createGraphQLServerOptions(req, context);
})(context, req, callbackFilter);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import {

describe('Memcached', () => {
const cache = new MemcachedCache('localhost');
afterAll(async () => {
await cache.close();
})
testKeyValueCache_Basics(cache);
testKeyValueCache_Expiration(cache);
});
71 changes: 43 additions & 28 deletions packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,53 @@
const IORedis = jest.genMockFromModule('ioredis');
class Redis {
private keyValue = {};
private timeouts = new Set<NodeJS.Timer>();

const keyValue = {};
async del(key: string) {
delete this.keyValue[key];
return true;
}

const deleteKey = key => {
delete keyValue[key];
return Promise.resolve(true);
};
async get(key: string) {
if (this.keyValue[key]) {
return this.keyValue[key].value;
}
}

const getKey = key => {
if (keyValue[key]) {
return Promise.resolve(keyValue[key].value);
async mget(...keys: string[]) {
return keys.map((key) => {
if (this.keyValue[key]) {
return this.keyValue[key].value;
}
});
}

return Promise.resolve(undefined);
};
async set(key, value, type, ttl) {
this.keyValue[key] = {
value,
ttl,
};
if (ttl) {
const timeout = setTimeout(() => {
this.timeouts.delete(timeout);
delete this.keyValue[key];
}, ttl * 1000);
this.timeouts.add(timeout);
}
return true;
}

nodes() {
return [];
}

const mGetKey = (key, cb) => getKey(key).then(val => [val]);
async flushdb() {}

const setKey = (key, value, type, ttl) => {
keyValue[key] = {
value,
ttl,
};
if (ttl) {
setTimeout(() => {
delete keyValue[key];
}, ttl * 1000);
async quit() {
this.timeouts.forEach((t) => clearTimeout(t));
}
return Promise.resolve(true);
};
}

IORedis.prototype.del.mockImplementation(deleteKey);
IORedis.prototype.get.mockImplementation(getKey);
IORedis.prototype.mget.mockImplementation(mGetKey);
IORedis.prototype.set.mockImplementation(setKey);
// Use the same mock as Redis.Cluster.
(Redis as any).Cluster = Redis;

export default IORedis;
export default Redis;
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import {
} from '../../../apollo-server-caching/src/__tests__/testsuite';

describe('Redis', () => {
const cache = new RedisCache({ host: 'localhost' });
const cache = new RedisCache();
afterAll(async () => {
await cache.close();
})
testKeyValueCache_Basics(cache);
testKeyValueCache_Expiration(cache);
});
7 changes: 6 additions & 1 deletion packages/apollo-server-caching/src/__tests__/testsuite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ export function testKeyValueCache_Expiration(
});

afterAll(() => {
jest.useRealTimers();
unmockDate();
keyValueCache.close && keyValueCache.close();
});

it('is able to expire keys based on ttl', async () => {
Expand Down Expand Up @@ -70,6 +70,11 @@ export function testKeyValueCache_Expiration(

export function testKeyValueCache(keyValueCache: TestableKeyValueCache) {
describe('KeyValueCache Test Suite', () => {
afterAll(async () => {
if (keyValueCache.close) {
await keyValueCache.close();
}
});
testKeyValueCache_Basics(keyValueCache);
testKeyValueCache_Expiration(keyValueCache);
});
Expand Down
4 changes: 4 additions & 0 deletions packages/apollo-server-cloud-functions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const server = new ApolloServer({
playground: true,
introspection: true,
});
// FIXME start

exports.handler = server.createHandler();
```
Expand Down Expand Up @@ -82,6 +83,7 @@ const server = new ApolloServer({
res,
}),
});
// FIXME start

exports.handler = server.createHandler();
```
Expand Down Expand Up @@ -111,6 +113,7 @@ const server = new ApolloServer({
typeDefs,
resolvers,
});
// FIXME start

exports.handler = server.createHandler({
cors: {
Expand Down Expand Up @@ -143,6 +146,7 @@ const server = new ApolloServer({
typeDefs,
resolvers,
});
// FIXME start

exports.handler = server.createHandler({
cors: {
Expand Down
13 changes: 0 additions & 13 deletions packages/apollo-server-cloud-functions/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,6 @@ export class ApolloServer extends ApolloServerBase {
}

public createHandler({ cors }: CreateHandlerOptions = { cors: undefined }) {
// We will kick off the `willStart` event once for the server, and then
// await it before processing any requests by incorporating its `await` into
// the GraphQLServerOptions function which is called before each request.
const promiseWillStart = this.willStart();

const corsHeaders = {} as Record<string, any>;

if (cors) {
Expand Down Expand Up @@ -129,14 +124,6 @@ export class ApolloServer extends ApolloServerBase {
}

graphqlCloudFunction(async () => {
// In a world where this `createHandler` was async, we might avoid this
// but since we don't want to introduce a breaking change to this API
// (by switching it to `async`), we'll leverage the
// `GraphQLServerOptions`, which are dynamically built on each request,
// to `await` the `promiseWillStart` which we kicked off at the top of
// this method to ensure that it runs to completion (which is part of
// its contract) prior to processing the request.
await promiseWillStart;
return this.createGraphQLServerOptions(req, res);
})(req, res);
};
Expand Down
6 changes: 5 additions & 1 deletion packages/apollo-server-cloudflare/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ export class ApolloServer extends ApolloServerBase {
}

public async listen() {
await this.willStart();
// In case the user didn't bother to call and await the `start` method, we
// kick it off in the background (with any errors getting logged
// and also rethrown from graphQLServerOptions during later requests).
this.ensureStarting();

addEventListener('fetch', (event: FetchEvent) => {
event.respondWith(
graphqlCloudflare(() => {
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-server-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@apollographql/apollo-tools": "^0.4.3",
"@apollographql/graphql-playground-html": "1.6.27",
"@apollographql/graphql-upload-8-fork": "^8.1.3",
"@josephg/resolvable": "^1.0.0",
"@types/ws": "^7.0.0",
"apollo-cache-control": "file:../apollo-cache-control",
"apollo-datasource": "file:../apollo-datasource",
Expand Down
Loading

0 comments on commit e552e3b

Please sign in to comment.