Errors in async iterators cause the websocket to close

[RMHonor]

When running a subscription operation, if the AsyncIterator errors, then it logs Internal error occurred during message handling..., and immediately closes the websocket.

I believe this is due to the onMessage handler for subscription operations not having any error handling: https://github.com/enisdenjo/graphql-ws/blob/master/src/server.ts#L799-L811

For example:

const resolvers: Resolvers = {
  Subscription: {
    notifications: {
      resolve: (source: any) => {
        if (source instanceof Error) {
          throw source;
        }

        return source;
      },
      subscribe: () => {
        return {
          async *[Symbol.asyncIterator]() {
            await new Promise((resolve) => setTimeout(resolve, 500));
            yield 'Hello world';
            // when this error is thrown, the whole websocket closes
            throw new Error('boom');
          },
        };
      },
    },
  },
};

I would've expected throwing the error in resolve would attach the error the operationResult.

Further Information
graphql-ws version: 5.6.2
node version: v14

[enisdenjo]

Are you sure it's the thrown error making problems and not your malformed yield? Please share the full error message.

const resolvers: Resolvers = {
  Subscription: {
    notifications: {
      resolve: (source: any) => {
        if (source instanceof Error) {
          throw source;
        }

        return source;
      },
      subscribe: () => {
        return {
          async *[Symbol.asyncIterator]() {
            await new Promise((resolve) => setTimeout(resolve, 500));
-           yield 'Hello world';
+           yield { notifications: 'Hello world' };
            // when this error is thrown, the whole websocket closes
            throw new Error('boom');
          },
        };
      },
    },
  },
};

[RMHonor]

The yield should be correct, the schema is:

extend type Subscription {
  notifications: String!
}

The error that occurs is:

Internal error occurred during message handling. Please check your implementation. Error: boom
    at Object.[Symbol.asyncIterator] (/Users/richardhonor/uw/crm-graphql/src/modules/notifications/notifications.resolver.ts:49:19)
    at AsyncGenerator.next (<anonymous>)
    at Object.next (/Users/richardhonor/uw/crm-graphql/node_modules/graphql/subscription/mapAsyncIterator.js:53:23)
    at onMessage (/Users/richardhonor/uw/crm-graphql/node_modules/graphql-ws/lib/server.js:207:165)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at WebSocket.<anonymous> (/Users/richardhonor/uw/crm-graphql/node_modules/graphql-ws/lib/use/ws.js:83:21)

I do get an initial message back to the client with "Hello world", so it is sending valid data back:

{
  "id": "8cbd7094-a307-4ae4-be0a-ec14a9b368e7",
  "type": "next",
  "payload": {
    "data": {
      "notifications": "Hello world"
    }
  }
}

But then when the error is thrown, the socket connection closes.

[enisdenjo]

Well then, if you expect errors thrown from subscribe to not bubble but instead change the operation result - the issue lies with graphql-js instead.

[enisdenjo]

Seems like this is intentional, please see graphql/graphql-js#1539.

[RMHonor]

Found the issue. When an error is thrown, the following line is not reached:

graphql-ws/src/server.ts

Line 822 in 45d1649

delete ctx.subscriptions[id];

This means that the ctx.subscriptions map continues to grow over time.

A workaround we've got for now is send a MessageType.Complete to the handler, which cleans up the subscription.

Perhaps the above line could be wrapped up in a try/finally, so the subscription is deleted?

[enisdenjo]

Great catch @RMHonor, thanks for pointing it out! I'll take care of it as soon as possible.

[enisdenjo]

Hey @RMHonor, fix for #333 (reply in thread) is now live with v5.8.2, try it out! 😄

[RMHonor]

Wow, quick work @enisdenjo, thanks a lot!

[aseemk]

For anyone else interested, I opened this issue with graphql-js to hopefully change this behavior:

graphql/graphql-js#4001

+1 reactions and/or any additional perspectives would be welcome! Thanks.

[enisdenjo]

Please refer to the recipe showcasing how someone may implement a custom subscribe for graceful handling thrown errors with just 15 lines of code.

[ralphstodomingo]

Just wanted to chime in - I followed that recipe, but it infinitely calls result.next, until the client disconnects, where I get a write EPIPE error.

In my case, trying to put done: true in the object returned within catch to stop the infinite looping, but TS expects me to return just a value field like the recipe. Also, the error doesn't balloon back to the client that way.

Another approach I tried was to return result.throw to let the async iterator that threw the error to clean up, but graphql-redis-subscriptions throws an error anyway in their async-iterator throw fn, so that still outputs Internal error occurred during message handling. Please check your implementation. for me, but at least it still lets the client to connect to the server that way.

[enisdenjo]

Hey! So, the thing is, .next can throw and have the iterator continue. The iterator stopping when .next throws is an assumption and hence an implementation detail. You're expected to implement a mechanism that breaks the iterator when .next throws yourself.

[ralphstodomingo]

I see! I'm not very familiar with the use of async iterators here, but what I did, where I had to invoke .throw manually in the recipe's catch is a valid way to have the iterator stop there as needed?