The server crashes if a promise with deferred handling rejects while another promise is trying to resolve

[mfanegg]

Issue Description

If an unhandled error and/or promise rejection occurs in a resolver, the expectation is that the error will be caught by the Apollo Server and added to an errors array in the response.

For example, this resolver:

...
const myResolver = async () => {
  return Promise.reject(new Error('I am rejecting.'));
}

const resolvers = {
  Query: {
    hello: myResolver
  },
};
...

should return something like this when queried:

{
  "data": {
    "hello": null
  },
  "errors": [
    {
      "message": "I am rejecting.",
...

However, "asynchronously handled" rejections (I'm not sure if my terminology is right) crash the server if the rejection occurs while another await is happening. Example:

// the crash only happens if REJECTION_TIMER_MILLIS < RESOLUTION_TIMER_MILLIS
const REJECTION_TIMER_MILLIS = 1000;
const RESOLUTION_TIMER_MILLIS = 3000;

const thisPromiseWillReject = () => {
  return new Promise((_, reject) => setTimeout(() => {
    reject(new Error('I am rejecting.'));
  }, REJECTION_TIMER_MILLIS));
}

const thisPromiseWillResolveAfterALongTime = () => {
  return new Promise((resolve) => setTimeout(resolve, RESOLUTION_TIMER_MILLIS));
}

const myResolver = async () => {
  const x = thisPromiseWillReject();
  await thisPromiseWillResolveAfterALongTime();
  await x;
}

const resolvers = {
  Query: {
    hello: myResolver
  },
};

It looks like the promise has to be rejected while another await is still pausing execution. If the rejection happens faster than the await right after it, then the error is handled as expected.

For further info, here is what my terminal looks like (my code is in a file called "server.mjs", and I'm using the simple standalone example from the apollo-server README):

mfan@mymachine mock-apollo-server-for-issue % node server.mjs
🚀 Server ready at http://localhost:4000/
file:///Users/mfan/dev/mock-apollo-server-for-issue/server.mjs:10
    reject(new Error('I am rejecting.'));
           ^

Error: I am rejecting.
    at Timeout._onTimeout (file:///Users/mfan/dev/mock-apollo-server-for-issue/server.mjs:10:12)
    at listOnTimeout (node:internal/timers:559:17)
    at processTimers (node:internal/timers:502:7)
mfan@mymachine mock-apollo-server-for-issue % 

I also have in this submission a link to a repo with the reproduction.

I was also able to repro this with .then syntax instead of async/await. This resolver will also crash the server:

const myResolverC = async () => {
  const x = thisPromiseWillReject();
  thisPromiseWillResolveAfterALongTime().then(() => {
    x.catch((err) => {
      console.log('caught error in resolver C')
    })
  });
}

I know that my repro code is not ideal -- promise rejections should be handled -- but I wouldn't expect this to take down the whole server. Could this be investigated?

Link to Reproduction

https://github.com/mfanegg/mock-apollo-server-for-issue-2

Reproduction Steps

1. clone the reproduction repo
2. make sure you're using node v16 (I haven't tried versions over 16 but maybe those will work)
3. in a terminal, run npm i
4. run node server.mjs
5. run the operation query { hello }
6. go to the terminal and observe that the server has exited

[glasser]

Just FYI, your package-lock.json seems to have been created with some internal corporate npm mirror so that the URLs it tries to install are all from a private mirror. I'll delete the package-lock.json file when trying to reproduce but that might mean I get a different version of packages!

[glasser]

I think this is basically working as intended, and it's not actually related to Apollo Server or GraphQL at all, just how Promises/async-await work in Node.JS.

When you write this code:

const myResolverC = async () => {
  const x = thisPromiseWillReject();
  await thisPromiseWillResolveAfterALongTime();
  await x;
}

there is no error handling connected to the x promise until the await x happens. As far as the JS runtime is concerned, nothing in the world is paying attention to the rejection of this Promise (yet!), and so any rejection is considered unhandled. Since Node v15 this will get translated (if you don't set an unhandledRejection handler) into an uncaught exception, which by default crashes your program.

Functions that call multiple Promise-returning functions should use a utility like Promise.all, Promise.allSettled, Promise.race, or Promise.any, all of which ensure that all of the Promises you pass to it get an appropriate error handler attached to it (or use .catch yourself making sure to .catch every Promise before waiting on any of them).

[glasser]

To demonstrate that this is unrelated to AS or graphql, replace your server.mjs with:

const REJECTION_TIMER_MILLIS = 1000;
const RESOLUTION_TIMER_MILLIS = 3000;

const thisPromiseWillReject = () => {
  return new Promise((_, reject) => setTimeout(() => {
    reject(new Error('I am rejecting.'));
  }, REJECTION_TIMER_MILLIS));
}

const thisPromiseWillResolveAfterALongTime = () => {
  return new Promise((resolve) => setTimeout(resolve, RESOLUTION_TIMER_MILLIS));
}

const myResolverC = async () => {
  const x = thisPromiseWillReject();
  await thisPromiseWillResolveAfterALongTime();
  await x;
}


try {
  await myResolverC();
} catch (e) {
  console.log(`Error caught from await: ${e}`)
}

Running this shows:

$ node server.mjs 
file:///Users/glasser/Projects/Apollo/mock-apollo-server-for-issue/server.mjs:6
    reject(new Error('I am rejecting.'));
           ^

Error: I am rejecting.
    at Timeout._onTimeout (file:///Users/glasser/Projects/Apollo/mock-apollo-server-for-issue/server.mjs:6:12)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)

Node.js v21.6.2

Note that you do not get the "Error caught from await" message here; this is the same issue, demonstrated without any use of @apollo/server or graphql.

[mfanegg]

Just FYI, your package-lock.json seems to have been created with some internal corporate npm mirror so that the URLs it tries to install are all from a private mirror. I'll delete the package-lock.json file when trying to reproduce but that might mean I get a different version of packages!

thanks for the heads up, I replaced the repro with a public one

[mfanegg]

@glasser I totally agree that uncaught exceptions should not exist in the app in the first place, but my concern comes from apollo-server being able to handle some uncaught exceptions, but crashing on others.

As you noted, the example I provided has an uncaught exception:

const myResolverC = async () => {
  const x = thisPromiseWillReject();
  await thisPromiseWillResolveAfterALongTime();
  await x;
}

because myResolverC is not handled until await happens. This turns into an uncaught exception as you noted, which crashes the app.

But this resolver is handled totally fine by apollo server:

const myFailingResolver = async () => {
  throw new Error('yikes');
}

Even though it's the same end result (uncaught exception). Maybe I'm missing something? It would be ideal if all unhandled exceptions were treated the same way by apollo-server.

[glasser]

My point is that Apollo Server has nothing to do with this Promise. The very thing that (in a sense) makes Apollo Server/graphql-js aware that this Promise is connected to the result of executing your resolver in any way is the fact that it gets a .catch or await applied to it while the function is running. As far as Apollo Server is concerned, this Promise might have been constructed in some completely unrelated piece of code, say some background setInterval or anything else. Whenever you create a Promise in JS, it's your job to tell Node that you're paying attention to rejections on that Promise before you yield to the event loop (by awaiting, say), not afterwards. This is what functions like Promise.all are for. Apollo Server doesn't somehow hijack the creation of Promises within resolvers to do something extra fancy to them to handle their rejections — you're expected to set your Promises up properly like in any other async code.

As far as Node knows, your function may have just been:

const myResolverC = async () => {
  const x = thisPromiseWillReject();
  await thisPromiseWillResolveAfterALongTime();
}

because it never actually gets to the point of running your await x before x rejects. And since there's no await here, it's not correct for myResolverC's returned Promise to reject just because x rejects — you haven't told the JS runtime to do that!

[mfanegg]

I see what the difference is now -- throw Error is picked up by the async resolver function as a promise rejection, but a promise with no await is just dangling in a sense so the async resolver function would never see it as you noted, and just resolve to undefined.

Thanks for explaining that to me, I will relay it.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.