facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.42k stars 1.83k forks source link

@live LiveState unsubscribe cleanup function never gets called #4830

Open subtleGradient opened 1 month ago

subtleGradient commented 1 month ago
/**
 * @RelayResolver Query.liveSubUnsubTest: Int
 * @live
 */
export function liveSubUnsubTest(): LiveState<number> {
  return {
    read() {
      console.log("liveSubUnsubTest", "read")
      return Date.now()
    },
    subscribe(didChange) {
      console.log("liveSubUnsubTest", "subscribe")
      const id = setInterval(() => {
        console.log("liveSubUnsubTest", "tick")
        didChange()
      }, 1234)
      return function unsubscribe() {
        console.log("liveSubUnsubTest", "unsubscribe")
        clearInterval(id)
      }
    },
  }
}
function LiveSubUnsubTestDemo() {
  const data = useClientQuery<LiveState__LiveSubUnsubTestDemo__Query>(
    graphql`
      query LiveState__LiveSubUnsubTestDemo__Query {
        liveSubUnsubTest
      }
    `,
    {},
  )

  if (!data.liveSubUnsubTest) {
    return <Text className="text-white">MIA</Text>
  }

  return (
    <>
      <Text className="text-white">
        {new Date(data.liveSubUnsubTest).toLocaleString()}
      </Text>
    </>
  )
}

after unmounting LiveSubUnsubTestDemo, I'm still getting the subscription ticks forever

 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick

console.log("liveSubUnsubTest", "unsubscribe") is never called

subtleGradient commented 1 month ago

Tried forcing root queries to expire ASAP with queryCacheExpirationTime: 1.

  const source = new RecordSource()
  const store = new RelayStore(source, {
    queryCacheExpirationTime: 1,
  })

It successfully causes __gc() to log zero marked references (indicating that nothing is being retained).

But the LiveState unsubscribe is still never called.

 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
 LOG  liveSubUnsubTest tick
subtleGradient commented 1 month ago

If references.size === 0, the entire _recordSource is cleared using _recordSource.clear().

This means that no records, including those with resolver subscriptions, will exist in the store. Consequently, the loop that iterates over storeIDs and checks for maybeResolverSubscription will never execute, and the subscriptions won't be disposed of properly.

subtleGradient commented 1 month ago

BOOM! Solved it.

If I comment out the references.size === 0 if branch, my LiveState is properly unsubscribed as expected.

      // if (references.size === 0) {
      //   console.debug('(RELAY)', 'RelayModernStore._collect clearing all records - no references');
      //   this._recordSource.clear();
      // } else {
...
      // }
subtleGradient commented 1 month ago

PROBLEM: This code bypasses maybeResolverSubscription

https://github.com/facebook/relay/blob/733cc273138564afae5e41fb37e77a77bb8e92b6/packages/relay-runtime/store/RelayModernStore.js#L726-L730

seems like we need to call maybeResolverSubscription sometimes

https://github.com/facebook/relay/blob/733cc273138564afae5e41fb37e77a77bb8e92b6/packages/relay-runtime/store/RelayModernStore.js#L738-L745

subtleGradient commented 1 month ago

quick and dirty workaround

  const source = new RecordSource()
  const store = new RelayStore(source, {
    // WARNING: DO NOT REMOVE THIS LINE
    // This is a workaround that unbreaks garbage collection in the Relay store
    // See https://github.com/facebook/relay/issues/4830 for more info
    // Without this, root queries will never be disposed and LiveState values will never unsubscribe
    queryCacheExpirationTime: 1,
  })
  const network = Network.create(fetchOrSubscribe, fetchOrSubscribe)
  const env = new Environment({ store, network })

  // WARNING: DO NOT REMOVE THIS LINE
  // This is a workaround that unbreaks garbage collection in the Relay store
  // See https://github.com/facebook/relay/issues/4830 for more info
  // RelayModernStore has two branches for clearing the store, one of which is broken
  // this line forces the store to use the working branch
  // by forcing the store to retain an extra root query
  // because the failing branch checks `if (references.size === 0)`
  env.retain(createOperationDescriptor(getRequest(graphql`query theRelayEnvironment__DummyQuery($id: ID!) { node(id:$id) { id } }`), { id: ROOT_ID })) // prettier-ignore
subtleGradient commented 1 month ago

related docs: #4831