fix(server): should not send error messages if socket closes before o…

…nSubscribe hooks resolves Closes #539
enisdenjo · Mar 27, 2024 · db47a66 · db47a66
1 parent f442288
commit db47a66
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 5 deletions.
diff --git a/src/__tests__/server.ts b/src/__tests__/server.ts
@@ -1795,6 +1795,65 @@ describe('Subscribe', () => {
       fail("Shouldn't have received a message");
     }, 20);
   });
+
+  it('should not send error messages if socket closes before onSubscribe hooks resolves', async () => {
+    let resolveOnSubscribe: () => void = () => {
+      throw new Error('On subscribe resolved early');
+    };
+    const waitForOnSubscribe = new Promise<void>(
+      (resolve) => (resolveOnSubscribe = resolve),
+    );
+
+    let resolveSubscribe: () => void = () => {
+      throw new Error('Subscribe resolved early');
+    };
+
+    const sendFn = jest.fn();
+
+    const closed = makeServer({
+      schema,
+      async onSubscribe() {
+        resolveOnSubscribe();
+        await new Promise<void>((resolve) => (resolveSubscribe = resolve));
+        return [new GraphQLError('Oopsie!')];
+      },
+    }).opened(
+      {
+        protocol: GRAPHQL_TRANSPORT_WS_PROTOCOL,
+        send: sendFn,
+        close: () => {
+          // noop
+        },
+        onMessage: async (cb) => {
+          await cb(stringifyMessage({ type: MessageType.ConnectionInit }));
+          await cb(
+            stringifyMessage({
+              id: '1',
+              type: MessageType.Subscribe,
+              payload: { query: '{ getValue }' },
+            }),
+          );
+        },
+        onPing: () => {
+          /**/
+        },
+        onPong: () => {
+          /**/
+        },
+      },
+      {},
+    );
+
+    await waitForOnSubscribe;
+
+    closed(4321, 'Bye bye!');
+
+    resolveSubscribe();
+
+    await new Promise((resolve) => setTimeout(resolve, 20));
+
+    expect(sendFn).toBeCalledTimes(1); // only the ack message
+  });
 });
 
 describe('Disconnect/close', () => {

diff --git a/src/server.ts b/src/server.ts
@@ -730,7 +730,9 @@ export function makeServer<
               const maybeExecArgsOrErrors = await onSubscribe?.(ctx, message);
               if (maybeExecArgsOrErrors) {
                 if (areGraphQLErrors(maybeExecArgsOrErrors))
-                  return await emit.error(maybeExecArgsOrErrors);
+                  return id in ctx.subscriptions
+                    ? await emit.error(maybeExecArgsOrErrors)
+                    : void 0;
                 else if (Array.isArray(maybeExecArgsOrErrors))
                   throw new Error(
                     'Invalid return value from onSubscribe hook, expected an array of GraphQLError objects',
@@ -760,17 +762,21 @@ export function makeServer<
                   execArgs.document,
                 );
                 if (validationErrors.length > 0)
-                  return await emit.error(validationErrors);
+                  return id in ctx.subscriptions
+                    ? await emit.error(validationErrors)
+                    : void 0;
               }
 
               const operationAST = getOperationAST(
                 execArgs.document,
                 execArgs.operationName,
               );
               if (!operationAST)
-                return await emit.error([
-                  new GraphQLError('Unable to identify operation'),
-                ]);
+                return id in ctx.subscriptions
+                  ? await emit.error([
+                      new GraphQLError('Unable to identify operation'),
+                    ])
+                  : void 0;
 
               // if `onSubscribe` didn't specify a rootValue, inject one
               if (!('rootValue' in execArgs))