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

@live LiveState unsubscribe cleanup function never gets called #4830

Open
subtleGradient opened this issue Oct 25, 2024 · 6 comments · May be fixed by #4832
Open

@live LiveState unsubscribe cleanup function never gets called #4830

subtleGradient opened this issue Oct 25, 2024 · 6 comments · May be fixed by #4832

Comments

@subtleGradient
Copy link

/**
 * @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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

subtleGradient commented Oct 25, 2024

PROBLEM: This code bypasses maybeResolverSubscription

// Sweep records without references
if (references.size === 0) {
// Short-circuit if *nothing* is referenced
this._recordSource.clear();
} else {

seems like we need to call maybeResolverSubscription sometimes

const maybeResolverSubscription = RelayModernRecord.getValue(
record,
RELAY_RESOLVER_LIVE_STATE_SUBSCRIPTION_KEY,
);
if (maybeResolverSubscription != null) {
// $FlowFixMe - this value if it is not null, it is a function
maybeResolverSubscription();
}

@subtleGradient
Copy link
Author

subtleGradient commented Oct 25, 2024

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
Copy link
Author

related docs: #4831

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

Successfully merging a pull request may close this issue.

1 participant