Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure ActionCableHandler only unsubscribes once #5109

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

rmosolgo
Copy link
Owner

Trying to fix #5086

@rmosolgo
Copy link
Owner Author

rmosolgo commented Oct 3, 2024

This didn't help :S

@rmosolgo rmosolgo closed this Oct 3, 2024
@vbatychko-modeln
Copy link
Contributor

@rmosolgo That's actually helps in my case, can you please reconsider? My case is: I'm using Urql for frontend, and when I'm doing unmount of one component that uses Urql.useSubscription, and rendering second component (switching components on the same page), that also uses Urql.useSubscription, unsubscribe happens twice.

Here is how it looks in websocket trace:

{"type":"welcome"}	18	14:54:18.554
{"command":"subscribe","identifier":"{\"channel\":\"GraphqlChannel\"}"}	71	14:54:18.554
{"identifier":"{\"channel\":\"GraphqlChannel\"}","type":"confirm_subscription"}	79	14:54:18.773
{"command":"message","identifier":"{\"channel\":\"GraphqlChannel\"}","data":"{\"query\":\"\\n subscription OnRefreshModel {\\n refreshModel {\\n model\\n state\\n updatedAt\\n }\\n}\\n \",\"variables\":{},\"action\":\"execute\"}"}	247	14:54:18.773
{"identifier":"{\"channel\":\"GraphqlChannel\"}","message":{"result":{"data":{}},"more":true}}	94	14:54:18.911
{"type":"ping","message":1730811262}	36	14:54:21.082
{"type":"ping","message":1730811265}	36	14:54:24.073
{"type":"ping","message":1730811268}	36	14:54:27.076
{"command":"unsubscribe","identifier":"{\"channel\":\"GraphqlChannel\"}"}	73	14:54:28.445
{"command":"subscribe","identifier":"{\"channel\":\"GraphqlChannel\"}"}	71	14:54:28.641
{"identifier":"{\"channel\":\"GraphqlChannel\"}","message":{"more":false}}	74	14:54:28.724
{"command":"unsubscribe","identifier":"{\"channel\":\"GraphqlChannel\"}"}	73	14:54:28.724
{"identifier":"{\"channel\":\"GraphqlChannel\"}","type":"confirm_subscription"}	79	14:54:28.780

This leave second component not subscribed. I ended up monkey patching createUrqlActionCableSubscription for fix.

import { Consumer, Subscription } from "@rails/actioncable"
type ForwardCallback = (...args: any[]) => void

function createUrqlActionCableSubscription(consumer: Consumer, channelName: string = "GraphqlChannel") {
    return function(operation: Operation) {
      let subscription: Subscription | null = null
      let subscribed = false

      return {
        subscribe: ({next, error, complete}: { next: ForwardCallback, error: ForwardCallback, complete: ForwardCallback}) => {
          subscription = consumer.subscriptions.create(channelName, {
            connected() {
              console.log(subscription)
              subscription?.perform("execute", { query: operation.query, variables: operation.variables }).then(() => {
                subscribed = true
              });
            },
            received(data: any) {
              if (data?.result?.errors) {
                error(data.errors)
              }
              if (data?.result?.data) {
                next(data.result)
              }
              if (!data.more) {
                complete()
              }
            }
          } as any)
          return {
            unsubscribe: () => {
                console.log('unsubscribe', subscribed)

                if (subscribed) {
                    subscribed = false
                    subscription?.unsubscribe()
                } else {
                    console.log('not subscribed')
                }
            }
          }
        }
      }
    }
  }
  
const forwardToActionCable = createUrqlActionCableSubscription(actionCable);

@rmosolgo rmosolgo reopened this Nov 5, 2024
@rmosolgo
Copy link
Owner Author

rmosolgo commented Nov 5, 2024

Hey @vbatychko-modeln, sure, I'd be happy to merge and release this -- thanks for sharing what you found!

@rmosolgo rmosolgo merged commit 9ab852f into master Nov 5, 2024
19 of 30 checks passed
@rmosolgo rmosolgo deleted the relay-action-cable-unsubscribe-once branch November 5, 2024 16:32
@rmosolgo
Copy link
Owner Author

rmosolgo commented Nov 5, 2024

🚢 in graphql-ruby-client v1.14.3. Please let me know if you have any more trouble with it!

@vbatychko-modeln
Copy link
Contributor

@rmosolgo I have spent some time debugging that issue with double unsubscriptions. Looks like the problem, at least in my case is as follows. Note that I'm using Urql with ActionCable.

Preconditions:

  1. Have one Urql.Client for React application in Urql.Provider
  2. Have 2 components, both are using the same subscription. E.g.,
    subscription OnMessageReceived {
    messgeReceived {
    message
    time
    }

The problem exposes itself, when first component is unmounting (that triggers also unsubscribe() call), and the second is starting subscription negotiation.

In this case, new handler for second component receives {more: false} AC message, which trigger complete() callback, and I suspect that complete() callback in turn calls unsubscribe(), causing subscription to interrupt negotiation. This leaves second component unsubscribed.

I have reworked initial monkey patch from my previous message to following, which fixes doubled unsubscription calls.
Well, at least, in my case.

import { Consumer, Subscription, logger } from "@rails/actioncable"
type ForwardCallback = (...args: any[]) => void

function createUrqlActionCableSubscription(consumer: Consumer, channelName: string = "GraphqlChannel") {
    return function (operation: Operation) {
        const subscribe = ({ next, error, complete }: { next: ForwardCallback, error: ForwardCallback, complete: ForwardCallback }) => {
            let subscribed = false;

            const subscription: Subscription = consumer.subscriptions.create(channelName, {
                connected() {
                    subscription.perform("execute", { query: operation.query, variables: operation.variables });
                    subscribed = true;
                },
                received(data: any) {
                    if (data?.result?.errors) {
                        error(data.errors);
                    }
                    if (data?.result?.data) {
                        next(data.result);
                    }
                    if (!data.more && subscribed) {
                        complete();
                    }
                }
            } as any);

            const unsubscribe = () => {
                if (subscribed) {
                    subscribed = false;
                    subscription?.unsubscribe();
                }
            };

            return { unsubscribe };
        };

        return { subscribe };
    };
}

const forwardToActionCable = createUrqlActionCableSubscription(actionCable);


const client = createClient({
    url: '/graphql',
    exchanges: [
        cacheExchange,
        fetchExchange,
        subscriptionExchange({
            forwardSubscription: forwardToActionCable as any
        }),
    ],
});

The change that matters at large is here:

                    if (!data.more && subscribed) {
                        complete();
                    }

I'm not sure how this translates to createActionCableHandler, which is targeted in this PR, but here are my 2c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to find subscription with identifier: {"channel":"GraphqlChannel"} (RuntimeError)
2 participants