apollo-server: close connections on ApolloServer.stop()

CHANGELOG.md

@@ -11,6 +11,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  With few exceptions, the format of the entry should follow convention (i.e., prefix with package name, use markdown `backtick formatting` for package names and code, suffix with a link to the change-set à la `[PR #YYY](https://link/pull/YYY)`, etc.).  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
+- `apollo-server`: Previously, `ApolloServer.stop()` functioned like `net.Server.close()` in that it did not close idle connections or close active connections after a grace period. This meant that trying to `await ApolloServer.stop()` could hang indefinitely if there are open connections. Now, this method closes idle connections, and closes active connections after 10 seconds. The grace period can be adjusted by passing the new `stopGracePeriodMillis` option to `new ApolloServer`, or disabled by passing `Infinity` (though it will still close idle connections). Note that this only applies to the "batteries-included" `ApolloServer` in the `apollo-server` package with its own built-in Express and HTTP servers. [ISsue #4097](https://github.com/apollographql/apollo-server/issues/4097)
 - `apollo-server-core`: When used with `ApolloGateway`, `ApolloServer.stop` now invokes `ApolloGateway.stop`. (This makes sense because `ApolloServer` already invokes `ApolloGateway.load` which is what starts the behavior stopped by `ApolloGateway.stop`.) Note that `@apollo/gateway` 0.23 will expect to be stopped in order for natural program shutdown to occur. [Issue #4428](https://github.com/apollographql/apollo-server/issues/4428)
 - `apollo-server-core`: Avoid instrumenting schemas for the old `graphql-extensions` library unless extensions are provided. [PR #4893](https://github.com/apollographql/apollo-server/pull/4893) [Issue #4889](https://github.com/apollographql/apollo-server/issues/4889)
 - `apollo-server-plugin-response-cache`: The `shouldReadFromCache` and `shouldWriteToCache` hooks were always documented as returning `ValueOrPromise<boolean>` (ie, that they could be either sync or async), but they actually only worked if they returned a bool. Now they can be either sync or async as intended. [PR #4890](http://github.com/apollographql/apollo-server/pull/4890) [Issue #4886](https://github.com/apollographql/apollo-server/issues/4886)

package.json

@@ -81,6 +81,7 @@
     "@types/qs-middleware": "1.0.1",
     "@types/request": "2.48.5",
     "@types/request-promise": "4.1.47",
+    "@types/stoppable": "1.1.0",
     "@types/supertest": "2.0.10",
     "@types/test-listen": "1.1.0",
     "@types/type-is": "1.6.3",

packages/apollo-server/package.json

@@ -26,7 +26,8 @@
     "apollo-server-express": "file:../apollo-server-express",
     "express": "^4.0.0",
     "graphql-subscriptions": "^1.0.0",
-    "graphql-tools": "^4.0.0"
+    "graphql-tools": "^4.0.0",
+    "stoppable": "^1.1.0"
   },
   "peerDependencies": {
     "graphql": "^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0"

packages/apollo-server/src/__tests__/index.test.ts

@@ -1,3 +1,4 @@
+import { createConnection } from 'net';
 import request from 'request';
 import { createApolloFetch } from 'apollo-fetch';
 
@@ -6,6 +7,7 @@ import { gql, ApolloServer } from '../index';
 const typeDefs = gql`
   type Query {
     hello: String
+    hang: String
   }
 `;
 
@@ -15,6 +17,19 @@ const resolvers = {
   },
 };
 
+class Barrier {
+  private resolvePromise!: () => void;
+  private promise = new Promise<void>((r) => {
+    this.resolvePromise = r;
+  });
+  async wait() {
+    await this.promise;
+  }
+  unblock() {
+    this.resolvePromise();
+  }
+}
+
 describe('apollo-server', () => {
   describe('constructor', () => {
     it('accepts typeDefs and resolvers', () => {
@@ -54,6 +69,65 @@ describe('apollo-server', () => {
       expect(fn.mock.calls).toEqual([['a'], ['b'], ['c'], ['d']]);
     });
 
+    describe('stops even with open HTTP connections', () => {
+      it('all connections are idle', async () => {
+        const server = new ApolloServer({
+          typeDefs,
+          resolvers,
+          // Disable killing non-idle connections. This means the test will only
+          // pass if the fast graceful close of the idle connection works.
+          stopGracePeriodMillis: Infinity,
+        });
+        const { port } = await server.listen({ port: 0 });
+
+        // Open a TCP connection to the server, and let it dangle idle
+        // without starting a request.
+        const connectionBarrier = new Barrier();
+        createConnection({ host: 'localhost', port: port as number }, () =>
+          connectionBarrier.unblock(),
+        );
+        await connectionBarrier.wait();
+
+        // Stop the server. Before, when this was just net.Server.close, this
+        // would hang. Now that we use stoppable, the idle connection is immediately
+        // killed.
+        await server.stop();
+      });
+
+      it('a connection with an active HTTP request', async () => {
+        const gotToHangBarrier = new Barrier();
+        const hangBarrier = new Barrier();
+        const server = new ApolloServer({
+          typeDefs,
+          resolvers: {
+            ...resolvers,
+            Query: {
+              ...resolvers.Query,
+              async hang() {
+                gotToHangBarrier.unblock();
+                await hangBarrier.wait(); // never unblocks
+              },
+            },
+          },
+          // A short grace period, because we're going to actually let this
+          // strike.
+          stopGracePeriodMillis: 10,
+        });
+        const { url } = await server.listen({ port: 0 });
+
+        // Start an HTTP request that won't ever finish. (Ignore the very
+        // expected error that happens after the server is stopped.)
+        const apolloFetch = createApolloFetch({ uri: url });
+        apolloFetch({ query: '{hang}' }).catch(() => {});
+        await gotToHangBarrier.wait();
+
+        // Stop the server. Before, when this was just net.Server.close, this
+        // would hang. Now that we use stoppable, the idle connection is immediately
+        // killed.
+        await server.stop();
+      });
+    });
+
     // These tests are duplicates of ones in apollo-server-integration-testsuite
     // We don't actually expect Jest to do much here, the purpose of these
     // tests is to make sure our typings are correct, and to trigger a

packages/apollo-server/src/index.ts

@@ -1,9 +1,11 @@
-// Note: express is only used if you use the ApolloServer.listen API to create
-// an express app for you instead of applyMiddleware (which you might not even
-// use with express). The dependency is unused otherwise, so don't worry if
-// you're not using express or your version doesn't quite match up.
+// This is the "batteries-included" version of `apollo-server-express`. It
+// handles creating the Express app and HTTP server for you (using whatever
+// version of `express` its dependency pulls in). If you need to customize the
+// Express app or HTTP server at all, you just use `apollo-server-express`
+// instead.
 import express from 'express';
 import http from 'http';
+import stoppable from 'stoppable';
 import {
   ApolloServer as ApolloServerBase,
   CorsOptions,
@@ -23,19 +25,22 @@ export interface ServerInfo {
 }
 
 export class ApolloServer extends ApolloServerBase {
-  private httpServer?: http.Server;
+  private httpServer?: stoppable.StoppableServer;
   private cors?: CorsOptions | boolean;
   private onHealthCheck?: (req: express.Request) => Promise<any>;
+  private stopGracePeriodMillis: number;
 
   constructor(
     config: ApolloServerExpressConfig & {
       cors?: CorsOptions | boolean;
       onHealthCheck?: (req: express.Request) => Promise<any>;
+      stopGracePeriodMillis?: number;
     },
   ) {
     super(config);
     this.cors = config && config.cors;
     this.onHealthCheck = config && config.onHealthCheck;
+    this.stopGracePeriodMillis = config?.stopGracePeriodMillis ?? 10000;
   }
 
   private createServerInfo(
@@ -113,13 +118,23 @@ export class ApolloServer extends ApolloServerBase {
     });
 
     const httpServer = http.createServer(app);
-    this.httpServer = httpServer;
+    // `stoppable` adds a `.stop()` method which:
+    // - closes the server (ie, stops listening)
+    // - closes all connections with no active requests
+    // - continues to close connections when their active request count drops to
+    //   zero
+    // - in 3 seconds (configurable), closes all remaining active connections
+    // - calls its callback once there are no remaining active connections
+    //
+    // If you don't like this behavior, use apollo-server-express instead of
+    // apollo-server.
+    this.httpServer = stoppable(httpServer, this.stopGracePeriodMillis);
 
     if (this.subscriptionServerOptions) {
       this.installSubscriptionHandlers(httpServer);
     }
 
-    await new Promise(resolve => {
+    await new Promise((resolve) => {
       httpServer.once('listening', resolve);
       // If the user passed a callback to listen, it'll get called in addition
       // to our resolver. They won't have the ability to get the ServerInfo
@@ -133,7 +148,7 @@ export class ApolloServer extends ApolloServerBase {
   public async stop() {
     if (this.httpServer) {
       const httpServer = this.httpServer;
-      await new Promise(resolve => httpServer.close(resolve));
+      await new Promise<void>((resolve) => httpServer.stop(() => resolve()));
       this.httpServer = undefined;
     }
     await super.stop();