Repository: graphql/graphql-js (ts)

Issue:
Unhandled rejection if an object has both a synchronous and asynchronous "Cannot return null for non-nullable field" error
Sorry about the title but it's a pretty specific scenario.

It looks like this error occurs because of how errors are handled for non-nullable fields. (I saw this with the `Cannot return null for non-nullable field` error, but it seems if the resolver throws an error it would be handled the same way.)

The problem appears to be here: https://github.com/graphql/graphql-js/blob/main/src/execution/execute.ts#L450
Let's say in the example below, the City object type has a resolver for `country`, which is asynchronous, and a resolver for `population`, which is synchronous, and they _both_ return null (which they shouldn't, of course). The resolver for the `country` field is run, and returns a promise. Then the resolver for the `population` field is run, and because it's synchronous, the error is thrown straight away, and the promise returned by the `country` resolver is discarded. When that promise eventually rejects, the process crashes with an `unhandledRejection` error. 

```js
// File: testcase.mjs
import { graphql, GraphQLInt, GraphQLList, GraphQLNonNull, GraphQLObjectType, GraphQLSchema, GraphQLString } from "graphql";

process.on("unhandledRejection", reason => {
    console.error("Unhandled rejection!", reason);
});

const City = new GraphQLObjectType({
    name: "City",
    fields: {
        name: { type: new GraphQLNonNull(GraphQLString) },
        country: { type: new GraphQLNonNull(GraphQLString), resolve: () => Promise.resolve(null) },
        population: { type: new GraphQLNonNull(GraphQLInt), resolve: () => null },
    }
});

const Query = new GraphQLObjectType({
    name: "Query",
    fields: {
        cities: {
            type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(City))),
            resolve() {
                return [{ name: "London", country: "GB", population: 8_982_000 }];
            }
        }
    }
});

const schema = new GraphQLSchema({
    types: [City],
    query: Query,
});

try {
    graphql(schema, `
        query {
            cities { name country population }
        }
    `).then(result => {
        if (result.errors)
            console.error("Error returned in result.errors[]:", result.errors);
        else
            console.log("Result:", result.data);
    }, error => {
        console.error("graphql() rejected:", error);
    });
} catch(err) {
    console.error("Error caught by try/catch:", err);
}
```

The output of this is:

```
Error returned in result.errors[]: [
  Error: Cannot return null for non-nullable field City.population.
      at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:566:13)
      at resolveField (/home/lee/restapi/node_modules/graphql/execution/execute.js:478:19)
      at executeFields (/home/lee/restapi/node_modules/graphql/execution/execute.js:292:18)
      at collectAndExecuteSubfields (/home/lee/restapi/node_modules/graphql/execution/execute.js:755:10)
      at completeObjectValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:745:10)
      at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:597:12)
      at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:563:21)
      at /home/lee/restapi/node_modules/graphql/execution/execute.js:627:25
      at Array.map (<anonymous>)
      at safeArrayFrom (/home/lee/restapi/node_modules/graphql/jsutils/safeArrayFrom.js:36:23) {
    locations: [ [Object] ],
    path: [ 'cities', 0, 'population' ]
  }
]
Unhandled rejection! Error: Cannot return null for non-nullable field City.country.
    at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:566:13)
    at /home/lee/restapi/node_modules/graphql/execution/execute.js:475:16 {
  locations: [ { line: 3, column: 27 } ],
  path: [ 'cities', 0, 'country' ]
}
```



Pull request description:
Fix crash in node when mixing sync/async resolvers
Fixes #3528

Ensures that if `executeFields` encounters a mix of promises and thrown errors, the promises will be awaited before throwing the error.

Patch:
diff --git a/src/execution/execute.ts b/src/execution/execute.ts
index 9f3cd4db..061a3ea7 100644
--- a/src/execution/execute.ts
+++ b/src/execution/execute.ts
@@ -624,23 +624,33 @@ function executeFields(
   const results = Object.create(null);
   let containsPromise = false;
 
-  for (const [responseName, fieldNodes] of fields) {
-    const fieldPath = addPath(path, responseName, parentType.name);
-    const result = executeField(
-      exeContext,
-      parentType,
-      sourceValue,
-      fieldNodes,
-      fieldPath,
-      asyncPayloadRecord,
-    );
+  try {
+    for (const [responseName, fieldNodes] of fields) {
+      const fieldPath = addPath(path, responseName, parentType.name);
+      const result = executeField(
+        exeContext,
+        parentType,
+        sourceValue,
+        fieldNodes,
+        fieldPath,
+        asyncPayloadRecord,
+      );
 
-    if (result !== undefined) {
-      results[responseName] = result;
-      if (isPromise(result)) {
-        containsPromise = true;
+      if (result !== undefined) {
+        results[responseName] = result;
+        if (isPromise(result)) {
+          containsPromise = true;
+        }
       }
     }
+  } catch (error) {
+    if (containsPromise) {
+      // Ensure that any promises returned by other fields are handled, as they may also reject.
+      return promiseForObject(results).finally(() => {
+        throw error;
+      });
+    }
+    throw error;
   }
 
   // If there are no promises, we can just return the object


Test patch:
diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts
index 628f71d1..a945b4d0 100644
--- a/src/execution/__tests__/executor-test.ts
+++ b/src/execution/__tests__/executor-test.ts
@@ -2,6 +2,7 @@ import { assert, expect } from 'chai';
 import { describe, it } from 'mocha';
 
 import { expectJSON } from '../../__testUtils__/expectJSON.js';
+import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js';
 
 import { inspect } from '../../jsutils/inspect.js';
 
@@ -580,6 +581,56 @@ describe('Execute: Handles basic execution tasks', () => {
     });
   });
 
+  it('handles sync errors combined with rejections', async () => {
+    let isAsyncResolverFinished = false;
+
+    const schema = new GraphQLSchema({
+      query: new GraphQLObjectType({
+        name: 'Query',
+        fields: {
+          syncNullError: {
+            type: new GraphQLNonNull(GraphQLString),
+            resolve: () => null,
+          },
+          asyncNullError: {
+            type: new GraphQLNonNull(GraphQLString),
+            async resolve() {
+              await resolveOnNextTick();
+              await resolveOnNextTick();
+              await resolveOnNextTick();
+              isAsyncResolverFinished = true;
+              return null;
+            },
+          },
+        },
+      }),
+    });
+
+    // Order is important here, as the promise has to be created before the synchronous error is thrown
+    const document = parse(`
+      {
+        asyncNullError
+        syncNullError
+      }
+    `);
+
+    const result = execute({ schema, document });
+
+    expect(isAsyncResolverFinished).to.equal(false);
+    expectJSON(await result).toDeepEqual({
+      data: null,
+      errors: [
+        {
+          message:
+            'Cannot return null for non-nullable field Query.syncNullError.',
+          locations: [{ line: 4, column: 9 }],
+          path: ['syncNullError'],
+        },
+      ],
+    });
+    expect(isAsyncResolverFinished).to.equal(true);
+  });
+
   it('Full response path is included for non-nullable fields', () => {
     const A: GraphQLObjectType = new GraphQLObjectType({
       name: 'A',