Fatal CloseEvent handling

[asmeikal]

I'm opening this as an issue, it may well be a question.

I'm currently in the process of migrating some GraphQL subscriptions services from subscriptions-transport-ws to graphql-ws. Our services are deployed with kubernetes, and I'm manually testing what happens when a service scales down while a client is connected to it.

The scale down seems to work fine with an Electron client: as expected, the graphql-ws client keeps trying to connect, until the service is scaled up again.

The scale down does not work as expected with two React Native clients (version 0.59 and 0.63). In both cases, when scaling down the service, the client is first disconnected with code 1001 and a "Going away" reason (the server is using something like this example). Then it tries to connect again, but this time fails with a non-standard error: it's something with a Expected HTTP 101 response but was '503 Service Unavailable' message, and no code or reason. There are no more connection attempts, I believe because of these checks in the client.

My question is: there is a way to recover gracefully from these types of errors? Gracefully for me in this context means without losing active subscriptions, ie, reestablishing them once the socket reconnects.

If it's not possible, I believe that the client should be allowed to choose how to handle these kinds of errors, and keep trying to connect.

[enisdenjo]

The Expected HTTP 101 response but was '503 Service Unavailable' is a HTTP error, not a WS error. They are all fatal in my opinion as the WS connection was never established to begin with.

What I would recommend is timing your retries using the retryWait client option. By controlling the promise resolution, you can make sure that the client retries when the server is ready. Like, for example, waiting for the server to become ready by polling a health-check.

My question is: there is a way to recover gracefully from these types of errors? Gracefully for me in this context means without losing active subscriptions, ie, reestablishing them once the socket reconnects.

A blunt answer is: no. The reason is that the client should retry exclusively on WS errors, because they can really only originate from within an existing connection. All other errors are fatal because there was no connection to begin with, or the error is too serious.

For example, the error you mentioned (Expected HTTP 101 response but was '503 Service Unavailable') cannot occur from within an existing connection - only when trying to establish a new one. In this case, timing your retries using a custom retry strategy is the way to go.

You might find the client usage with custom retry timeout strategy recipe helpful.

[asmeikal]

Hi @enisdenjo, I don’t believe that checking if the server is up in the retryWait is safe enough: it could still happen that the retryWait is successful, and the reconnection attempt fails for whatever reason (network problems, service crashed between the two calls, or other errors that React Native doesn’t handle properly). Or a load balancer may direct my “health check” request to an instance, and the actual connection to another (faulty) instance.

I personally believe that allowing for a “while true keep trying” logic is an inevitable choice in a production application, and not doing so burdens the developer with a lot of complexity. If, for example, a reconnection fails for a similar error, or an internal server error caused by a buggy deploy, the developer would have to handle the complexity of restoring all subscriptions that were created before the connection error, or crash the client which is terrible UX. On the other hand, moving this to the retryWait function means that the developer has to identify and handle all possible “bad situations”, some of which (e.g., the buggy deploy) may only be detected after actually attempting to connect.

Anyway, I believe that this falls way out of the scope of the actual protocol, and is an implementation detail of the client. I’m considering implementing an opinionated client for this protocol, since this and other issues (like the client ping) are too subjective. The important thing is that the protocol works! 😄

[enisdenjo]

I follow. Your arguments make sense.

One key element that I overlooked is that the connection interrupt might occur with a "safe" WS close event, but the server becomes unavailable for retries. This is exactly the HTTP "503 Service Unavailable" tries to communicate.

Also, you're completely right with the argument of retryWait being unreliable. It can also happen that the check itself stalls for whatever reason, rendering the client completely useless because it is stuck.

A simple retry is "kind of" fail proof.

That being said, I will see if I can add a convenient way of allowing the user to decide if the client should retry.

On the other note, I am trying to keep graphql-ws as small and as simple as possible, which is why I am hard to sway. However, I am completely in favor of the community, given enough senseful arguments - this library can climb extreme heights.

Feel free to approach the implementation your way, Id love to see how others take up the task! I hope this lib inspires progress and movement. 😄

[enisdenjo]

Hey hey, @asmeikal, what do you think about #126? Returning false from isFatalConnectionProblem always will deem all connection errors as retryable.

[asmeikal]

Hi @enisdenjo, thanks! I think that it would work! But I think that some of the errors (1002 and all the 44XX) should still and always be fatal for the client, since they are explicitly part of the protocol and indicate either a bug in the client (44XX) or a client/server misconfiguration (1002).

[enisdenjo]

You're right, that makes sense. I'll keep the current set of fatal close codes as forever fatal, but will allow the implementor to control others.

[enisdenjo]

🎉 This issue has been resolved in version 4.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[loucadufault]

(Sorry for comment on an older issue)

Wanted to confirm the understanding of retry logic.

Based on this code, passing shouldRetry: () => true will cause the client to retry, except for a fatal CloseEvent or non-CloseEvent connection problem, as defined here. Assuming of course the two earlier checks pass (there remain retry attempts and all not subscriptions completed).

[enisdenjo]

Yes, that is right.