Lambda: onHealthCheck handling doesn't stop normal request processing

[pontusvision]

This bug report should include:

• A short, but descriptive title. The title doesn't need "Apollo" in it.
• The package name and version of Apollo showing the problem:

apolo-server-lambda: 2.12.0

• If applicable, the last version of Apollo where the problem did not occur.

n/a

• The expected behaviour

when registered (by calling ApolloServer.createHandler(), the callback should be invoked whenever an HTTP request is made to /.well-known/apollo/server-health.

• The actual behaviour:

Because of a bug in the implementation, the call gets executed, but the handler function carries on running, and interprets the server-health request as a normal GQL query, or as a Playground request. The problem is here in the ApolloServer.createHandler() method:

        if (event.path === '/.well-known/apollo/server-health') {
        const successfulResponse = {
          body: JSON.stringify({ status: 'pass' }),
          statusCode: 200,
          headers: {
            'Content-Type': 'application/json',
            ...requestCorsHeadersObject,
          },
        };
        if (onHealthCheck) {
      /// >>>>>>  THIS FUNCTION NEVER RETURNS <<<<<<<<<<
          onHealthCheck(event)
            .then(() => {
              return callback(null, successfulResponse);
            })
            .catch(() => {
              return callback(null, {
                body: JSON.stringify({ status: 'fail' }),
                statusCode: 503,
                headers: {
                  'Content-Type': 'application/json',
                  ...requestCorsHeadersObject,
                },
              });
            });
          } else {
            return callback(null, successfulResponse);
          }
      }

• A simple, runnable reproduction!
  Please make a GitHub repository that anyone can clone and run and see the
  problem. Other great ways to demonstrate a reproduction are using our
  CodeSandbox template (https://codesandbox.io/s/apollo-server), or re-mixing
  our Glitch template (https://glitch.com/~apollo-launchpad).

To reproduce, instantiate apollo server, and set a simple 'hello world' function:

const apolloServer = new ApolloServer();
apolloServer.createHandler(
  { cors: null, 
  onHealthCheck: (event: APIGatewayProxyEvent)=> {
    console.log(`got event ${JSON.stringify(event)}`); 
  return true;
  });

Then run a bash script to call the health check:

while [[ true ]]; do curl -v -XGET -H 'Accept: application/json' -H 'Content-Type: application/json' 'http://localhost:4000/.well-known/apollo/server-health'; done

Proposed solution:

My proposed solution (after spending about 3 days trying to troubleshoot what was going on in lambdas) is to convert the handler function to use the more modern 'async/await' pattern:

class ApoloServer{
   ...

  public createHandlerHC(
    { cors, onHealthCheck }: CreateHandlerOptions = {
      cors: undefined,
      onHealthCheck: 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 = new Headers();

    if (cors) {
      if (cors.methods) {
        if (typeof cors.methods === "string") {
          corsHeaders.set("access-control-allow-methods", cors.methods);
        } else if (Array.isArray(cors.methods)) {
          corsHeaders.set(
            "access-control-allow-methods",
            cors.methods.join(",")
          );
        }
      }

      if (cors.allowedHeaders) {
        if (typeof cors.allowedHeaders === "string") {
          corsHeaders.set("access-control-allow-headers", cors.allowedHeaders);
        } else if (Array.isArray(cors.allowedHeaders)) {
          corsHeaders.set(
            "access-control-allow-headers",
            cors.allowedHeaders.join(",")
          );
        }
      }

      if (cors.exposedHeaders) {
        if (typeof cors.exposedHeaders === "string") {
          corsHeaders.set("access-control-expose-headers", cors.exposedHeaders);
        } else if (Array.isArray(cors.exposedHeaders)) {
          corsHeaders.set(
            "access-control-expose-headers",
            cors.exposedHeaders.join(",")
          );
        }
      }

      if (cors.credentials) {
        corsHeaders.set("access-control-allow-credentials", "true");
      }
      if (typeof cors.maxAge === "number") {
        corsHeaders.set("access-control-max-age", cors.maxAge.toString());
      }
    }

    return async (
      event: APIGatewayProxyEvent,
      context: LambdaContext
      // callback: APIGatewayProxyCallback
    ) => {
      // We re-load the headers into a Fetch API-compatible `Headers`
      // interface within `graphqlLambda`, but we still need to respect the
      // case-insensitivity within this logic here, so we'll need to do it
      // twice since it's not accessible to us otherwise, right now.
      const eventHeaders = new Headers(event.headers);

      // Make a request-specific copy of the CORS headers, based on the server
      // global CORS headers we've set above.
      const requestCorsHeaders = new Headers(corsHeaders);

      if (cors && cors.origin) {
        const requestOrigin = eventHeaders.get("origin");
        if (typeof cors.origin === "string") {
          requestCorsHeaders.set("access-control-allow-origin", cors.origin);
        } else if (
          requestOrigin &&
          (typeof cors.origin === "boolean" ||
            (Array.isArray(cors.origin) &&
              requestOrigin &&
              cors.origin.includes(requestOrigin)))
        ) {
          requestCorsHeaders.set("access-control-allow-origin", requestOrigin);
        }

        const requestAccessControlRequestHeaders = eventHeaders.get(
          "access-control-request-headers"
        );
        if (!cors.allowedHeaders && requestAccessControlRequestHeaders) {
          requestCorsHeaders.set(
            "access-control-allow-headers",
            requestAccessControlRequestHeaders
          );
        }
      }

      // Convert the `Headers` into an object which can be spread into the
      // various headers objects below.
      // Note: while Object.fromEntries simplifies this code, it's only currently
      //       supported in Node 12 (we support >=6)
      const requestCorsHeadersObject = Array.from(requestCorsHeaders).reduce<
        Record<string, string>
      >((headersObject, [key, value]) => {
        headersObject[key] = value;
        return headersObject;
      }, {});

      if (event.httpMethod === "OPTIONS") {
        context.callbackWaitsForEmptyEventLoop = false;
        return {
          body: "",
          statusCode: 204,
          headers: {
            ...requestCorsHeadersObject
          }
        };
        // return callback(null, {
        //   body: '',
        //   statusCode: 204,
        //   headers: {
        //     ...requestCorsHeadersObject,
        //   },
        // });
      }

      if (event.path === "/.well-known/apollo/server-health") {
        const successfulResponse = {
          body: JSON.stringify({ status: "pass" }),
          statusCode: 200,
          headers: {
            "Content-Type": "application/json",
            ...requestCorsHeadersObject
          }
        };

        const failureResponse = {
          body: JSON.stringify({ status: "fail" }),
          statusCode: 503,
          headers: {
            "Content-Type": "application/json",
            ...requestCorsHeadersObject
          }
        };

        if (onHealthCheck) {
          // wait HERE!!! Otherwise, the apollo server execution carries on!!!
          // return;
          const hcRes = await onHealthCheck(event);

          return hcRes ? successfulResponse : failureResponse;
          // .then(() => {
          //   // return callback(null, successfulResponse);
          //   return successfulResponse;
          // })
          // .catch(() => {
          //   return callback(null, {
          //     body: JSON.stringify({ status: 'fail' }),
          //     statusCode: 503,
          //     headers: {
          //       'Content-Type': 'application/json',
          //       ...requestCorsHeadersObject,
          //     },
          //   });
          // });
        } else {
          return successfulResponse;
          // return callback(null, successfulResponse);
        }
      }

      if (this.playgroundOptions && event.httpMethod === "GET") {
        const acceptHeader = event.headers["Accept"] || event.headers["accept"];
        if (acceptHeader && acceptHeader.includes("text/html")) {
          const path =
            event.path ||
            (event.requestContext && event.requestContext.path) ||
            "/";

          const playgroundRenderPageOptions: PlaygroundRenderPageOptions = {
            endpoint: path,
            ...this.playgroundOptions
          };

          // return callback(null, {
          return {
            body: renderPlaygroundPage(playgroundRenderPageOptions),
            statusCode: 200,
            headers: {
              "Content-Type": "text/html",
              ...requestCorsHeadersObject
            }
          };
          // );
        }
      }

      return this.handleGQL(
        event,
        context,
        promiseWillStart,
        requestCorsHeaders
      );

      // return graphqlLambda()
      // const callbackFilter: APIGatewayProxyCallback = (error, result) => {
      //   callback(
      //     error,
      //     result && {
      //       ...result,
      //       headers: {
      //         ...result.headers,
      //         ...requestCorsHeadersObject
      //       }
      //     }
      //   );
      // };

      // graphqlLambda(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(event, context);
      // })(event, context, callbackFilter);
    };
  }

  private async handleGQL(
    event: APIGatewayProxyEvent,
    context: LambdaContext,
    promiseWillStart: Promise<void>,
    requestCorsHeaders: Headers
  ) {
    context.callbackWaitsForEmptyEventLoop = false;
    const optionsFunc = 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(event, context);
    };
    if (event.httpMethod === "POST" && !event.body) {
      return {
        body: "POST body missing.",
        statusCode: 500
      };
    }
    const corsHeaders = Array.from(requestCorsHeaders).reduce<
      Record<string, string>
    >((headersObject, [key, value]) => {
      headersObject[key] = value;
      return headersObject;
    }, {});
    // let headers = new Headers(event.headers);
    // headers.append (requestCorsHeadersObject);
    const eventHeaders = Array.from(new Headers(event.headers)).reduce<
      Record<string, string>
    >((headersObject, [key, value]) => {
      headersObject[key] = value;
      return headersObject;
    }, {});

    try {
      const gqlResp: HttpQueryResponse = await runHttpQuery([event, context], {
        method: event.httpMethod,
        options: optionsFunc,
        query:
          event.httpMethod === "POST" && event.body
            ? JSON.parse(event.body)
            : event.queryStringParameters,
        request: {
          url: event.path,
          method: event.httpMethod,
          headers: new Headers(event.headers)
        }
      });

      const combinedHeaders = new Headers({
        ...corsHeaders,
        ...eventHeaders,
        ...gqlResp.responseInit.headers
      });

      return {
        body: gqlResp.graphqlResponse,
        statusCode: 200,
        headers: combinedHeaders
      };
    } catch (error) {
      if ("HttpQueryError" !== error.name) {
        return error;
      }
      const combinedHeaders = new Headers({
        ...corsHeaders,
        ...eventHeaders,
        ...error.headers
      });

      return {
        body: error.message,
        statusCode: error.statusCode,
        headers: combinedHeaders
      };
    }
  }

}

[uehondor]

+1

I've also found that handler doesn't wait on the promise returned from the onHealthCheck callback.

I think a simple return in this code will suffice.

For example:

[glasser]

I agree that this looks like a bug in the Lambda health check handler that you can escape from the health check block and keep going.

Note that @adikari in #1989 is discussing refactoring the Lambda handler to work either as async (Promise-returning) or callback-taking, which may conflict with this work.

[uehondor]

@glasser Hi David, thanks for bringing this to my attention. I agree the lambda handler needs refactoring.
I think a reactor at that scale might take longer to get in, merged and released with the backlog of open PRs.

I wondered if the simple bug fix above might be easier to get merged and released to unblock teams.

[glasser]

Yep, I'd also take a PR for that!

[glasser]

Fixed in #4969

[glasser]

I've published a release with this to version 2.21.1-alpha.0. My plan is to release 2.21.1 on Monday. If you want to test that this fix works before then, try running the prerelease yourself, and provide feedback on #4990

[adikari]

I have updated the changes in this #4969 slightly to support changes for #5004