Problem in unsubscribe of useSubscription react hook

[reza-ebrahimi]

When react component unmounts, the useSubscription hook doesn't unsubscribe stream in server side.

const TestComponent = () => {
  const { loading, error, data } = useSubscription(SUBSCRIPTION);
  
  useEffect(() => {
    return () => {
      // unsubscribe here
    };
  }, []);

  return <div>TestComponent</div>;
};

[zrcni]

useSubscription already cleans up after itself :broom: :wink: https://github.com/apollographql/apollo-client/blob/5eb624c08ac6bc6ddfa93607793242cebd75f5b1/src/react/hooks/useSubscription.ts#L41

[reza-ebrahimi]

@zrcni I know that, But it doesn't unsubscribe the stream in server side. Note that everything works fine with Graphql playgorund but not working using useSubscription hook. Using the hook, stream not stopping or destroying in server side and still continue to stream values.

[reza-ebrahimi]

I have created a test project for both client and server, Check it out: https://github.com/reza-ebrahimi/TestSubscrtiptionBug

[reza-ebrahimi]

It seems useSubscription hook sends start request twice, but stop is sent only once. See this: https://github.com/async-graphql/async-graphql/issues/478#issuecomment-819148931

Thanks @sunli829

[ghempton]

We are seeing this in our application as well with lots of leaked subscriptions when using the useSubscription hook. This seems like a pretty fundamental issue, has anyone taken a look?

[Venryx]

I was wondering why the "stop" event was not being sent to my server, and it seems this is why!

Seems like an issue, since it could potentially keep dozens of subscriptions open that are not actually used by the client anymore. (eg. if the user is moving between lots of different pages, each with their own sets of subscriptions)

[Venryx]

So I've looked into this a bit more, and the path that is taken from the user calling subscription.unsubscribe to the actual sending of the MessageTypes.GQL_STOP message is kinda complicated. One could argue that the complexity is needed for the level of abstraction sought -- and maybe that is true -- but it certainly makes debugging this issue difficult.

Here is the path that is taken:

1. User calls useSubscription: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/react/hooks/useSubscription.ts#L15
2. When the component unmounts, unsubscribe is called (on the observable stored in the hook): https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/react/hooks/useSubscription.ts#L120 2.1) That subscription variable was set here: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/react/hooks/useSubscription.ts#L89 2.2) That observable variable was set here: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/react/hooks/useSubscription.ts#L28-L39 2.3) That client.subscribe function is defined here: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/core/ApolloClient.ts#L374-L378 2.4) Which calls this function: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/core/QueryManager.ts#L847 2.5) (which returns this unsubscribe function in the next step)
3. Then this unsubscribe function is called: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/core/QueryManager.ts#L900
4. Which calls unsubscribe on an observable wrapper, created either here: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/core/QueryManager.ts#L992-L994 ... or here ... https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/core/QueryManager.ts#L992-L994
5. Either way, that observable wrapper appears to then have an unsubscribe method on it, because it inherits it from the base class Observable, which is actually just a reexport of Observable from zen-observable-ts, which is a reexport of the Observable class here: https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L198

I unfortunately could not definitively locate the definition of the unsubscribe function. But I think it ends up resolving to this unsubscribe method in the base Observable class: https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L182-L187

Which calls closeSubscription: https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L84 ...and cleanupSubscription: https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L59

The latter of which appears to call this cleanup function here (back up in the Concast class): https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/utilities/observables/Concast.ts#L216-L239

If you read through the cleanup function above, we might have a piece of the puzzle: as the comment mentions, the Concat.cleanup method does not unsubscribe from the underlying observable.

Now the comment makes it sound like it's not the cleanup function's role to do that unsubscribing.

But the issue is that nowhere in the Concast class do I see anything that calls unsubscribe (other than an error handler, which may never get called). And the class is indeed the one that is calling subscribe on the underlying observables: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/utilities/observables/Concast.ts#L207-L210

But maybe the base Observable class is supposed to be calling unsubscribe on the underlying observables? Maybe.

There are some lines in it that call unsubscribe (though who knows in what circumstances): https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L229 https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L239 https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L345 https://github.com/zenparsing/zen-observable/blob/328fb66a99242900c0fd4a330e9ff66ecfa3a887/src/Observable.js#L390-L391

At this point, this is getting too complex for me to follow, so I've given up for now. If someone could continue the investigation, it would be appreciated. (perhaps one of the apollo-client maintainers?)

---

For anyone wanting to take that journey, I think this is the "finish line" where you want the unsubscribe calls to end up: https://github.com/apollographql/subscriptions-transport-ws/blob/11f24136a5e39c6d218d8486a9abff1219a4a565/src/client.ts#L674

That SubscriptionClient class from subscriptions-transport-ws appears to be instantiated in this apollo-client file: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/link/ws/index.ts#L42

And each request (which starts a subscription) passes through that WebSocketLink class' request method here: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/link/ws/index.ts#L50

Which is called by ApolloLink.execute, defined here: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/link/core/ApolloLink.ts#L70

Which is called by those lines referenced earlier (these two blocks) in QueryManager.ts that instantiate those Concat classes.

So from the above, we have a rough idea of what the pathway is; but it was not clear to me from reading through it exactly where the issue is.

That said, my guess is that the problem is somewhere in either apollo-client's Concast.ts or zen-observable's Observable.js.

[Venryx]

I was about to read through the test files to check whether there are tests set up (to confirm that unsubscribe is being called on subscriptions when they ought to be dropped), but then I opened this: https://github.com/apollographql/apollo-client/blob/a565fd5036b23810f59b49affc69a36cdb434a55/src/core/tests/QueryManager/index.ts

And I saw it has 5,881 lines.

And I quietly backed away.

[benjamn]

Based on all the great details @Venryx shared above, I believe I've made some progress on this problem in PR #9718. Please take a look when you have a chance! Running npm i @apollo/client@beta will give you version 3.7.0-alpha.4 which includes the changes.

[webatom]

@benjamn Hey, I have the same problem

Upgrading to 3.7.0-beta.1 did not help :(

[benjamn]

@Venryx Did PR #9718 make any difference for you? You can run npm i @apollo/client@beta to try those changes.

[Venryx]

@Venryx Did PR #9718 make any difference for you? You can run npm i @apollo/client@beta to try those changes.

I'm a bit confused tbh, because just now I went to check the dev-tools WebSocket events for my website, and I am not currently seeing the issue I saw earlier, of the "stop" event never being sent.

My guess is that, for my website, the issue came about due to some higher-level issue that ended up getting resolved from the other changes I've made in the last few months. So unfortunately, I can't confirm whether your patch resolves my original issue.

That said, I notice three people upvoted the comment above saying the problem still exists for them. Maybe one of them has a reproduction repository that they can share?

[felixmccuaig]

Having the same issue here: Subscriptions not re-subbing when device is locked :((

[tetchel]

for what it's worth since I found this through google - though there are mixed opinions online (eg. https://github.com/apollographql/apollo-client/issues/7964#issuecomment-817801295, this SO post) about whether or not useSubscription cleans up after itself, it was not doing this for me. I log my listeners server-side and noticed them accumulating even though those components had unmounted.

In my use-case I was using subscribeToMore. I fixed the cleanup problem by calling subscribeToMore in useEffect and then using its return value as the cleanup function.

useEffect(() => {
  const unsub = subscribeToMore({ ... your options })
  return () => unsub()
})

I don't know why the docs don't mention this. https://www.apollographql.com/docs/react/data/subscriptions/#subscribing-to-updates-for-a-query

using apollo-client 3.6.9

[MrCox007]

Same issue, useSubscriptions is not resubbing after device is locked. I use expo with apollo-client v3 and hasura as backend.

[jazithedev]

I experience the same problem 🙁. Refreshing React app with subscriptions active doesn't send stop event to backend.

[alessbell]

It seems useSubscription hook sends start request twice, but stop is sent only once. See this: https://github.com/async-graphql/async-graphql/issues/478#issuecomment-819148931

As far as I can tell, this would only ever occur when used in combination with React's StrictMode, the issue described in https://github.com/apollographql/apollo-client/issues/6037.

@jazithedev @reza-ebrahimi @ghempton @Venryx would anyone be able to either corroborate that StrictMode is involved in your cases, or provide a runnable reproduction?

We have a CodeSandbox and forkable GitHub repo with a subscriptions.tsx already configured for subscriptions reproductions. Thanks!

[jmarthernandez]

@alessbell I'm also seeing this error outside of strict mode. In my case I have added debug logs to make sure the component is unmounted but I still get updates from the unmounted component.

There seems to be a problem with the subscription endpoint(https://uifesi.sse.codesandbox.io/graphql) so I can't try to recreate the scenario in the sandbox.

[HelaGone]

Hello. Any updates on this? I have a similar problem with the socket not closing its connection after the user closes or reloads the screen. The problem I'm facing is that those loose connections are piling up in the ApolloServer until the point where ApolloServer struggles to respond requests and fails. Is there a way to handle loose connections from ApolloServer?

[phryneas]

The next minor release of Apollo Client will come with a rewrite of useSubscription, so the problem might be solved by that - if it's a problem we are actually able to address.

What @HelaGone mentions here with subscriptions staying open after a screen reload seems like it would be outside of our control here - are you talking a full app restart/browser refresh?

[HelaGone]

Yes it is on full reload when I see that the socket never closes/completes. However I've seen that many other sites that use web sockets heavily have the same behavior. That's what makes me wonder if those loose connections should be managed by the server instead?

[phryneas]

On a full reload, the browser will just stop all JavaScript on the site, so there's not much "disconnecting" logic we can add, I fear.

That said, it's also up to the browser to close open connections, so either your browser has a bug (which is very unlikely), or the connection is closed correctly, but for some reason your server doesn't register that. I'd investigate your server here.