unhandled promise rejection

[joshribakoff]

In my resolver I return a native Promise, in the executor function I access a variable that was not declared. When I make requests from graphiql that invoke my resolver, I get errors in my terminal about unhandled promise rejections & my http request is left spinning (unterminated). After 60s I get a 504 timeout from nginx which I have reverse proxying traffic to my nodeJS process. Using express-graphql 0.6.11 with graphql-tools 1.2.3 to separate my schema & resolvers.

This is undesirable default for a graphQL server implementation over HTTP behavior because easy to miss mistakes result in a degraded performance on the frontend where the user is waiting maybe 60s for nginx to time out before eventually getting feedback in the UI that something went wrong. Instead, I expect a graphQL implementation to terminate the user's http request as soon as a failure is known, ideally null out that field in the response with graphQL errors response, and rethrow the error (so still trigger an unhandled promise rejection or some kind of error I can catch in a higher context).

I'd be surprised if there isn't also some sort of memory leak / attack surface here, since requests are left sitting open & seemingly never terminated.

[IvanGoncharov]

@joshribakoff graphql-tool is built on top of graphql-js but it also adds some custom logic so I'm not sure that you issue related to this lib. I would suggest you to report this issue to graphql-tools repo.

[leebyron]

We have some pretty comprehensive test coverage for returning Promises within the executor, but it's always entirely possible we haven't covered some case.

@joshribakoff Could you provide some minimal failing test case?

[joshribakoff]

Here is a minimalist example that replicates the problem, I actually also have graphql-errors in addition to graphql-tools in the play, I realized. I will try to narrow it down more & post back with more details.

I cloned my demo repo from when I first learned graphQL:

https://github.com/joshribakoff/graphql-demo

I added this resolver to replicate the issue:

module.exports = {
  Query: {
    random:  new Promise(async (resolve, reject) => { foo.bar() }),
  }
}

Here is the issue itself:

Joshs-MacBook-Air:graphql-demo josh$ yarn start
yarn run v1.1.0
warning ../package.json: No license field
$ node index.js
Running a GraphQL API server at localhost:4000/graphql
(node:3793) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): ReferenceError: foo is not defined
(node:3793) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Also have you considered about pending backend requests? promises inherently have no cancellation like an observable. I have seen instances where graphQL is pegging backend services even though the client is gone. It just seems like there's some resources not being cleaned up somewhere but I can't put my finger on it.

Edit: come to think of it looks funky passing an async arrow as the executor, that is probably the main issue there, either way even if the code is "wrong" is there not some way for graphQL to time out requests & clean up resources in the presence of programming mistakes like these?

[leebyron]

Also have you considered about pending backend requests?

Cancellation of pending backend requests to a connection is a general problem beyond GraphQL, but I think whatever solution the community agrees upon will be happily adopted. I see promise in cancellation tokens (https://github.com/tc39/proposal-cancellation) and abort controllers (https://developers.google.com/web/updates/2017/09/abortable-fetch) - both of which can be implemented in user space (and supplied via GraphQL context).

[leebyron]

There are a couple problems with your resolver that is resulting in the issue you're seeing.

module.exports = {
  Query: {
    random:  new Promise(async (resolve, reject) => { foo.bar() }),
  }
}

First, resolvers should be provided as functions, which a Promise is not. GraphQL.js should be asserting this during construction, however graphql-tools might not be giving you the right error messages since I believe it mutates an existing schema (cc @stubailo)

Secondly, and to get to the root of the issue you're seeing, is that an async function produces a Promise when called, but the new Promise() constructor expects a function which it will eagerly call and it does not do anything with the return value. I can replicate your issue without any GraphQL packages at all:

$ node
> new Promise(async (resolve, reject) => { foo.bar() }).catch(err => console.log('Caught:' + err))
Promise {}
> (node:86531) UnhandledPromiseRejectionWarning: ReferenceError: foo is not defined

If you're looking to use the new Promise() constructor, just use a normal function. If you're looking to provide an async function, don't wrap it with new Promise(). Here are two variations of your code which should allow GraphQL.js to properly handle the error during execution:

module.exports = {
  Query: {
    // note: Takes a function which returns a Promise, Promise constructed with normal function
    random:  () => new Promise((resolve, reject) => { foo.bar() }),
  }
}

Or:

module.exports = {
  Query: {
    // note: Takes an async function (which is a function that returns a Promise)
    random:  async () => { foo.bar() },
  }
}

[joshribakoff]

I think whatever solution the community agrees upon

Which community? I see packages like rxjs, redux-overservable & angular are already using observables, which are conceptually a super-set of promises. I'm not just talking about canceling the backend requests initiated in the executor function, but also the resources consumed by a promise that is kept around in memory that may never resolve or reject... and pending http requests that do not get terminated.

First, resolvers should be provided as functions, which a Promise is not.

My bad. My resolver was a function which returned a promise. I accidentally typed the example up wrong here & left that out. The issue still happens even with a function that returns a promise.

the new Promise() constructor expects a function which it will eagerly call and it does not do anything with the return value.

Correct, the fact it disregards the inner returned promise should be inconsequential since I do not care about that promise, I will await something & then call resolve() on the outer promise is my intention.

If you're looking to use the new Promise() constructor, just use a normal function. If you're looking to provide an async function, don't wrap it with new Promise()

FYI the use case is that lots of libraries do not work with async/await, for example in my experience if you instrument a popular library called Puppeteer & register a .on('error', () => {}) callback and attempt to throw an error, we would expect that to be caught by try/catch, however errors thrown in the context of another library's callback do not get caught by try/catch in some cases, which as you said is a short coming of the language features, hence the need to mix promise constructors and async functions... It does seem like a language/ecmascript runtime issue though, hence why I was suggesting observables instead of promises, which would be a community supported way [rxjs has 18k stars on github] to bring in cancelation.

In your second example it fails silently, if inside the async fn I wrap in try/catch some code that instruments Puppeteer & register an "on error" callback fn, and inside of the callback's fn I throw an new Error, I expect the "catch" to catch the error, but again it creates an unhandled promise rejection.

I will workaround it on my end by wrapping libraries to promisify, and then await those promises inside of async functions. I really wish someone would do something though, nodeJS ought to terminate my script or something so I don't just have users potentially waiting 60s for a request to time out, unbeknownst to me.

[leebyron]

Which community?

The general JavaScript community. Promises are a native feature of the language and runtime while Observables are not. GraphQL.js seeks to interact smoothly with the JavaScript language. Most Observable libraries support a toPromise() API that should allow them to be converted to Promises should you prefer to use Observables in your backing code. Since the discussion around AbortControllers is fairly new, I would expect rxjs and others to be discussing how to incorporate it. I see one such discussion is already underway.

I understand the use case of Promise constructors for converting non-Promise APIs to be Promise based - my suggestion is to ensure that you use async functions and Promise constructors together in the way they are intended to avoid issues like the one you encountered.

[joshribakoff]

Ok thank you, just for anyone reading this who is confused & facing the same issue, the correct way to mix them is probably something like this:

myResolver: async() => {
        try {
            await new Promise(resolve, reject => {
                setTimeout(reject, 1000)
            })
        } catch(err) {
            console.error(err)
            return null
        }
    }

It just stinks a little that if you accidentally wrap them the other way around you get such drastic consequences, I guess that is more of an upstream issue that should be filed with TC39 or something, I feel like if its unintended then it should be an error, but obviously this is unrelated to graphQL at this point, thanks again for your time.