Race condition in useSubscription causing 'loading' to remain true

[davetapley]

Intended outcome:

When a subscription's initial data is returned by the GraphQL server,
the useSubscription hook should:

• Set loading to false.
• Provide the data in data.
• Do this 100% of the time.

Actual outcome:

Approximately 30% of the time (in my case) the initial data is returned by the GraphQL server but loading remains true and data remains undefined.

How to reproduce the issue:

Based on my investigate I believe this issue will occur in any application which uses useSubscription. However due to its intermittent / race condition nature it will be highly dependent on the complexity of the React application, and network conditions.

I have posted a screen cast here demonstrating the issue here.

Root cause:

As identified in the video, I believe the root cause is a race condition in /packages/hooks/src/data/SubscriptionData.ts which implicitly requires that isMounted will be set to true before updateResult is called, and this is not always the case.

I haven't investigated further, but it's possible this is also the root cause for:
#3774 #3488 #3361 #3425 #3090 #2899

Version

  System:
    OS: macOS Mojave 10.14.6
  Binaries:
    Node: 10.15.3 - ~/.nvm/versions/node/v10.15.3/bin/node
    npm: 6.10.3 - ~/.nvm/versions/node/v10.15.3/bin/npm
  Browsers:
    Chrome: 79.0.3945.117
    Firefox: 72.0.1
    Safari: 13.0.4
  npmPackages:
    @apollo/react-common: 3.1.3 => 3.1.3 
    @apollo/react-hoc: 3.1.3 => 3.1.3 
    @apollo/react-hooks: 3.1.3 => 3.1.3 
    apollo-boost: 0.4.7 => 0.4.7 
    apollo-cache-inmemory: 1.6.5 => 1.6.5 
    apollo-client: 2.6.8 => 2.6.8 
    apollo-link: 1.2.13 => 1.2.13 
    apollo-link-context: 1.0.19 => 1.0.19 
    apollo-link-http: 1.5.16 => 1.5.16 
    apollo-link-ws: 1.0.19 => 1.0.19 
    react-apollo: 3.1.3 => 3.1.3 

[davetapley]

I forgot to mention in the video: There is a workaround:

The skip option can be used to prevent the subscription being started until isMounted has been set to true, thus:

  const [skip, setSkip] = useState<boolean>(true);
  const { data, error, loading } = useSubscription({skip, ...});
  useEffect(() => {
    setSkip(false);
..

[0xdevalias]

Probably related: #3424

I was having a world of troubles even with this workaround, so ended up attempting to write my own useSubscription hook. It didn't necessarily work any better.. but in case it's useful for anyone:

const useSubscriptionCustom = ({ query, variables }) => {
  const apolloClient = useApolloClient()

  const [subscription, setSubscription] = useState(null)
  const [subscriptionConnected, setSubscriptionConnected] = useState(false)

  // const subscriptionRef = useRef(null)

  const onNext = useCallback(result => {
    setSubscription({ error: null, ...result })
  }, [])

  const onError = useCallback(error => {
    setSubscription({ data: null, error })
  }, [])

  const onComplete = useCallback(() => {
    // console.log('sub.subscribe.3: Finished')
    setSubscriptionConnected(false)
  }, [])

  useEffect(() => {
    const subscriptionObservable = apolloClient.subscribe({
      query,
      variables,
      fetchPolicy: 'no-cache',
    })

    const sub = subscriptionObservable.subscribe(onNext, onError, onComplete)
    // subscriptionRef.current = sub

    setSubscriptionConnected(true)

    // console.error('custom useSubscription', sub)

    return () => {
      sub.unsubscribe()
      setSubscriptionConnected(false)
    }
  }, [apolloClient, query, variables])

  // if (subscriptionRef.current) {
  //   const closed = subscriptionRef.current.closed || true
  //   const currentConnectedStatus = !closed
  //
  //   console.error({ closed, currentConnectedStatus, subscriptionConnected })
  //
  //   if (subscriptionConnected !== currentConnectedStatus) {
  //     setSubscriptionConnected(currentConnectedStatus)
  //   }
  // }

  return {
    connected: subscriptionConnected,
    ...subscription,
  }
}

Usage:

const subscription = useSubscriptionCustom({
    query: RECEIVED_COMMAND_SUBSCRIPTION,
    variables: useMemo(() => ({ channelID: 'foo' }), []),
  })