(retry) - Allow retryIf to avoid retrying subscriptions on non-network error

[gabor]

i use both retry-exchange and subscriptions. if there is an error on the server, while generating a subscription-event, then the error correctly arrives to the client, retry-middleware does a retry, and from this point on, useSubscription will be triggered multiple times for the same event.

urql version & exchanges:
urql=1.10.2 or 1.10.3
retry-exchange=0.1.10

Steps to reproduce

i created a codesandbox-example, this is the server:
https://codesandbox.io/s/urql-sub-retry-problem-server-6rrhd (nothing special here)
this is the client:
https://codesandbox.io/s/urql-sub-retry-problem-client-h8ie7

open the client, open the url with the running example, and open devtools, watch the console-output. (do not leave this running too long, or the tab might become unresponsive)
the server is set up so that it generates a new event every second, and has a 60% chance to throw while doing so.
while you get no errors, the new events arrive correctly, but after the first error happens, retry happens, and things get to arrive duplicated. when more errors happen, it becomes even more duplicated.

if you change the codesandbox example, to switch urql to version 1.10.1, then it is suddenly fixed. it now works correctly.

unfortunately, in my local codebase reverting to urql=1.10.1 does not fix the problem. i do not understand why, maybe a combination of package-versions?

Expected behavior
actually i am not even sure what exactly should happen, a subscription-event cannot really be retried. but duplicate events should not happen.

Actual behavior
i am getting duplicate events.

[kitten]

Hm, I believe it may depend strongly on the transport layer.

By default the retryExchange will only retry an operation when it has a networkError (on the CombinedError that is) https://github.com/FormidableLabs/urql/blob/4b01dc1165c8f734175398ea4edf45139d0ba2d0/exchanges/retry/src/retryExchange.ts#L43

Typically a subscription will only throw this once the entire observable is broken, so if it errors out: https://github.com/FormidableLabs/urql/blob/4b01dc1165c8f734175398ea4edf45139d0ba2d0/packages/core/src/exchanges/subscription.ts#L84

So far I believe that's the correct behaviour. In your example I'm noticing however that you've set this to always be true for testing behaviour.

I think it's a matter of perspective on whether the retryExchange should even take subscription operations into account, but enabling it to do so does enable more use-cases.

However we could expose the entire operation on retryIf so that those can be fully ignored maybe?

[gabor]

hmm, i wasn't able to write a short reply, so i made a list :-)

1. being able to see if it's subscription/mutation/query in retryIf would be nice, so i could choose different behavior for subscriptions
2. until [1] gets implemented, can i do some exchange "routing" magic to do things differently for subscriptions and differently for query/mutation?
3. for retrying subscriptions, what i'd like to achieve is this: when the subscription "starts", and it fails there, i want to retry that. when it is already "active", and it fails during generating/receiving the event, i do not want to retry that (it is not really possible to receive an event from the past anyway). i never want to be "double subscribed". now that i think about it more, subscription-transport-ws already has a reconnect:true option. will that "fix" a failed subscription "start", or only redo the websocket transport?
4. hmm.. if retry-exchange will retry a "start subscription" thing, shouldn't it close the maybe-existing "started subscription" thing? i'm not sure if this is possible to do though.
5. i actually do like retryIf: () => true even in production for queries and mutations, as long as i can make them idempotent.
6. maybe there should be a warning "somewhere" if someone tries do do retry + retryIf=>true + subscription? i'm not sure where, but it is always incorrect to do it this way, right? because you will leak the subscriptions.
7. do you know why switching between versions 1.10.1 and 1.10.2 produces different results?

[kitten]

and it fails during generating/receiving the event, i do not want to retry that (it is not really possible to receive an event from the past anyway)

It still comes back to the original problem here. If you do retryIf: () => true you're reacting to GraphQL Errors, which are not recoverable and shouldn't trigger a retry unless you know that the specific errors are retry-able. https://formidable.com/open-source/urql/docs/basics/errors/

Most GraphQL Errors come from the schema directly and are predictable and repeatable because they're thrown by the API explicitly, e.g. for authentication or another reason.

for retrying subscriptions, what i'd like to achieve is this: when the subscription "starts", and it fails there

It's not expected to. If anything, that would probably a network error, but subscriptions don't really differentiate between a start and end for most transport protocols.

i'm not sure where, but it is always incorrect to do it this way, right?

I'd argue the default is most correct here which only reacts to network errors, and even for queries retrying on everything is unfortunately also not quite safe for queries 😅

do you know why switching between versions 1.10.1 and 1.10.2 produces different results?

Nothings changed to subscriptions in a long time, including these versions. It's probably worth mentioning though that it's up to @urql/core's version on which version of the built-in exchanges are running.

So I think, still, in summary, the API addition makes sense, but I still want to caution you here, because I believe all exchanges here are behaving correctly, so the edge case basically comes down to your implementation of retryIf. That being said, we do want to enable this use-case so some Operation input would be nice. I'd still say though retrying on each error is likely not desirable 😅 However I believe the subscriptionExchange is behaving as expected here

[gabor]

thanks for the fast response here. i will re-think the retry-handling in my code to make it better.

[kitten]

Published in @urql/exchange-retry@0.2.0