Skip to content

Commit

Permalink
draft: async server.start() function
Browse files Browse the repository at this point in the history
Full description to come, but tl;dr:

Previously server startup worked like this:

- AS constructor runs
- 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 not await)
  protected willStart, 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:
- There's an explicit state machine situation inside AS
- AS constructor initializes state with schema directly if no gateway
- If there is a 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.

Overall changes:
- If you're using `apollo-server`, startup errors will cause `listen` to reject,
  nothing else necessary.
- If you're using an integration you are encouraged to call `await
  server.start()` yourself after the constructor, which will let you detect
  startup errors.
- But if you don't do that, the server will still start up with similar
  properties. gateway.load won't start until the integration's `ensureStarting`
  or graphqlServerOptions' `ensuresStarted` though. Errors will be logged.
- Also if you don't call `server.start()`, the full stack trace of any startup
  error will be logged on *every* failed graphql request instead of just a short
  message suggesting there's more in logs (but it's only at the beginning of
  your logs)

Yes this is the tl;dr version that I jotted off at the end of my work day off
the top of my head without going back and skimming through the PR for details :)

Fixes #4921. Fixes apollographql/federation#335.
  • Loading branch information
glasser committed Mar 3, 2021
1 parent 44a6641 commit f2241d7
Show file tree
Hide file tree
Showing 27 changed files with 641 additions and 473 deletions.
9 changes: 4 additions & 5 deletions packages/apollo-server-azure-functions/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ 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();
// 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();

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

Expand Down Expand Up @@ -145,7 +145,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
16 changes: 4 additions & 12 deletions packages/apollo-server-cloud-functions/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ 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();
// 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();

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

Expand Down Expand Up @@ -129,14 +129,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
Loading

0 comments on commit f2241d7

Please sign in to comment.