Fix crash in node when mixing sync/async resolvers

Fixes #3528

Author: asztal

src/execution/__tests__/executor-test.ts

@@ -625,6 +625,60 @@ describe('Execute: Handles basic execution tasks', () => {
     });
   });
 
+  it('handles sync errors combined with rejections', async () => {
+    const catchUnhandledRejections = (reason: any) => {
+      throw new Error('Unhandled rejection: ' + reason.message);
+    };
+
+    process.on('unhandledRejection', catchUnhandledRejections);
+
+    try {
+      const schema = new GraphQLSchema({
+        query: new GraphQLObjectType({
+          name: 'Type',
+          fields: {
+            syncNullError: {
+              type: new GraphQLNonNull(GraphQLString),
+              resolve: () => null,
+            },
+            asyncNullError: {
+              type: new GraphQLNonNull(GraphQLString),
+              resolve: () => Promise.resolve(null),
+            },
+          },
+        }),
+      });
+
+      // Order is important here, as the promise has to be created before the synchronous error is thrown
+      const document = parse(`
+        {
+          asyncNullError
+          syncNullError
+        }
+      `);
+
+      const rootValue = {};
+
+      const result = await execute({ schema, document, rootValue });
+      expectJSON(result).toDeepEqual({
+        data: null,
+        errors: [
+          {
+            message:
+              'Cannot return null for non-nullable field Type.asyncNullError.',
+            locations: [{ line: 3, column: 11 }],
+            path: ['asyncNullError'],
+          },
+        ],
+      });
+
+      // Allow time for the asyncReject field to be rejected
+      await new Promise((resolve) => setTimeout(resolve, 50));
+    } finally {
+      process.off('unhandledRejection', catchUnhandledRejections);
+    }
+  });
+
   it('Full response path is included for non-nullable fields', () => {
     const A: GraphQLObjectType = new GraphQLObjectType({
       name: 'A',

src/execution/execute.ts

@@ -444,34 +444,53 @@ function executeFields(
 ): PromiseOrValue<ObjMap<unknown>> {
   const results = Object.create(null);
   let containsPromise = false;
+  let error: Error | undefined;
 
   for (const [responseName, fieldNodes] of fields.entries()) {
     const fieldPath = addPath(path, responseName, parentType.name);
-    const result = executeField(
-      exeContext,
-      parentType,
-      sourceValue,
-      fieldNodes,
-      fieldPath,
-    );
+    try {
+      const result = executeField(
+        exeContext,
+        parentType,
+        sourceValue,
+        fieldNodes,
+        fieldPath,
+      );
 
-    if (result !== undefined) {
-      results[responseName] = result;
-      if (isPromise(result)) {
-        containsPromise = true;
+      if (result !== undefined) {
+        results[responseName] = result;
+        if (isPromise(result)) {
+          containsPromise = true;
+        }
       }
+    } catch (err) {
+      error = err;
+    }
+
+    if (error) {
+      break;
     }
   }
 
   // If there are no promises, we can just return the object
   if (!containsPromise) {
-    return results;
+    if (error) {
+      throw error;
+    } else {
+      return results;
+    }
   }
 
   // Otherwise, results is a map from field name to the result of resolving that
   // field, which is possibly a promise. Return a promise that will return this
   // same map, but with any promises replaced with the values they resolved to.
-  return promiseForObject(results);
+  const promise = promiseForObject(results);
+  if (error) {
+    // Ensure that any promises returned by other fields are handled, as they may also reject.
+    return promise.then(() => Promise.reject(error));
+  }
+
+  return promise;
 }
 
 /**