Double subscriptions when using useTracker in lazy components

[ksinas]

When using useTracker in lazy components I noticed that sometimes data get subscribed, unsubscribed and subscribed again.

This happens because useMemo and useEffect sometimes gets called synchronously, thus comp.stop() gets called too late, resulting in second subscription.

[CaptainN]

useMemo and useEffect (given the same set of deps) should not ever be called synchronously. Can you point me to some information about cases where it might?

A reproduction or unit test would be even better.

I wonder if the useEffect hook is getting called before Meteor.defer gets a chance to run for some other reason in some cases.

[CaptainN]

A simple rewrite could help (would need testing):

  const memoizedComp = useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
        if (c.firstRun) data = refs.reactiveFn();
      })
    );
    if (Meteor.isDevelopment) {
      checkCursor(data);
    }
    return comp;
  }, deps);

  // This won't run in concurrency mode - leaving more errant subscriptions laying around...
  useLayoutEffect(() => {
    // To avoid creating side effects in render, stop the computation immediately
    memoizedComp.stop();
    // We might also need to set data here, not ideal
  }, [memoizedComp]);

  useEffect(() => {
    const computation =  Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        setData(refs.reactiveFn(c));
      })
    );
    return () => {
      computation.stop();
    }
  }, deps);

In addition to adding another hook, it also might have a bug where the data state gets lost after deps change (would need to test that). It might be possible to put the clean-up in the useEffect block, but then we might have to defer the creation of the next computation - things are getting complicated again.

Oh! Right, this won't work, because it would leave the computation laying around in concurrency mode...

Update: Actually, that data state loss issue is already addressed by another change, which you can find on the comparator branch: https://github.com/meteor/react-packages/blob/e30e2905f8bcfca095f5929ef9888b061b39584a/packages/react-meteor-data/useTracker.ts#L111

[CaptainN]

Anyway, I can fix this if needed, but I'll need a reproduction.

[ksinas]

Actually, the problem is with Meteor.defer. I replicated it and wrote a simple app which shows the sequence of actions. You can see that they are called in different sequence when using a lazy component https://codesandbox.io/s/delicate-hooks-61067?file=/src/App.js

[ksinas]

What if something like this? If we avoid stopping initial computation, then we would also preserve the same output from the first computation. Is there any negative consequence to this approach?

const useTracker = (reactiveFn, deps) => {
  let [data, setData] = useState();
  const { current: refs } = useRef({ reactiveFn, mounted: false, computation: null });
  refs.reactiveFn = reactiveFn;
  refs.data = data;

  useMemo(() => {
    refs.computation = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
       // keep the outorun alive, but do not create side effects in render
        refs.intermediateData = refs.reactiveFn();
        if (Meteor.isDevelopment) {
          checkCursor(refs.intermediateData);
        }
        if (c.firstRun) {
          refs.data = refs.intermediateData
          delete refs.intermediateData;
        } else if (refs.mounted) {
          setData(refs.intermediateData)
          delete refs.intermediateData;
        }
      })
    );
  }, deps);

  useEffect(() => {
    refs.mounted = true;
    if (refs.intermediateData) {
      setData(refs.intermediateData)
      delete refs.intermediateData;
    } else {
      setData(refs.data)
    }
    return () => {
      refs.mounted = false;
      refs.computation.stop()
      refs.computation = null;
    }
  }, deps);
  return refs.data;
}

[CaptainN]

Interesting. Replacing meteorDefer with setTimeout fixes your test case. But I don't know if that's a workable solution in production, because if I understand it correctly, browser limit the smallest setTimeout interval to 4ms, and I don't know how long it takes in minimum for useEffect to run after render.

I can come up with a solution without using setTimeout, but it's a bummer that Meteor.defer doesn't work in this case. Maybe this is something that can be fixed on that side?

[ksinas]

I wouldn't change it to setTimeout. You could just always stop the computation on the start of the useEffect. But how about my previous suggestion? It solves 2 issues instead.

[CaptainN]

Your suggestion makes 100% sense, except when concurrent mode is used, which is the real problem with all this. Your computation.stop() would never be called for 3 out of 4 renders created when using concurrent mode, because in that mode the render body is called multiple times, but useEffect is only called once on the instance that gets "committed" (sometimes we say "mounted").

Meteor.subscribe actually has a ton of internal details that make it super hard to choreograph all this too. It has it's own internal lifecycle, and if it's not catered to just right, you get churn in your pub/subs.

We used to do a ton of work to keep the initial computation from first render alive, using timeouts, and checking things in useEffect - but it was horrendously difficult to maintain, and there were still many edge cases where it'd fail (in some cases causing subscriptions to die, and other to churn in a subscribe/unsubscribe death loop).

I'm actually thinking this is a bug in Meteor.defer - can you confirm whether it's ever run? I think it's not actually getting run in this case - that could be big bug in Meteor.defer that should be fixed on that side of this.

[CaptainN]

Actually, maybe we don't even need that defer? We might be able to take a page out of useTrackerNoDeps and simply call stop inline when the component hasn't yet been "mounted". I'll play around with that later.

[ksinas]

I'm not exactly sure how concurrent mode works under the hood, but I would be surprised that it would evaluate useMemo multiple times with the same deps. That sounds like a potential performance issue.

[CaptainN]

It definitely does - useMemo is just run inline with the rest of the render, and if the render is suspended, it's values are discarded. (I've seen some assertions that useRef is shared between renders in concurrent mode - but I don't think it is.) The individual "concurrently" rendered passes do not share any state between them.

[ksinas]

It definitely does - useMemo is just run inline with the rest of the render, and if the render is suspended, it's values are discarded. (I've seen some assertions that useRef is shared between renders in concurrent mode - but I don't think it is.) The individual "concurrently" rendered passes do not share any state between them.

I played a bit around and can confirm that indeed, useMemo is called multiple times and refs are not shared. So that means that in concurrent mode might be multiple subscribe / unsubscribe happening. I guess it's safer to do subscriptions purely in useEffect

[CaptainN]

@ksinas Some things changed in 2.3.1 can you confirm whether or not this is still an issue in 2.3.1?

Also, please take a look at this forum thread, which seems to talk about similar things. Basically, a sub-unsub-sub cycle might not be a problem, because Meteor does a bunch of stuff internally to protect against various bad outcomes (which I've worked to support correctly in useTracker/withTracker).

I want to make sure what you have described here isn't something else though.

[mfen]

I wonder if the useEffect hook is getting called before Meteor.defer gets a chance to run for some other reason in some cases.

@CaptainN I'm seeing this exact behaviour with 2.3.1, but without lazy components. Stepping through I can confirm that the useEffect() runs before the Meteor.defer() eventually runs.

We might be able to take a page out of useTrackerNoDeps and simply call stop inline when the component hasn't yet been "mounted". I'll play around with that later.

Did you have a chance to try this out? If it is problematic, perhaps a last-resort guard of comp.stop() at the start of the useEffect in case defer has not yet run?

The problem is compounded for us as we are using cult-of-coders/grapher with scoped queries, such that the doubled subscription triggers an additional DDP changed message for every published doc with the second _sub_<handle.subscriptionId> scope field.

[CaptainN]

I think I need a test for this, so I can make sure we get it solved in a reliable way.

Removing Meteor.defer breaks a bunch of tests. It could be something in the tests, but I have to figure it out before we move forward with removing that.

[CaptainN]

I think this will work - it doesn't add much overhead to the hook, and will definitely make sure things happen in the right sequence.:

const useTrackerWithDeps = <T = any>(reactiveFn: IReactiveFn<T>, deps: DependencyList, skipUpdate: ISkipUpdate<T> = null): T => {
  const forceUpdate = useForceUpdate();

  const { current: refs } = useRef<{
    reactiveFn: IReactiveFn<T>;
    data?: T;
    comp?: Tracker.Computation;
  }>({ reactiveFn });

  // keep reactiveFn ref fresh
  refs.reactiveFn = reactiveFn;

  useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
        refs.data = refs.reactiveFn();
      })
    );
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => { comp.stop() });
    // In some cases, the useEffect hook will run before Meteor.defer, such as
    // when React.lazy is used. This will allow it to be stopped earlier in
    // useEffect if needed.
    refs.comp = comp;
  }, deps);

  useEffect(() => {
    refs.comp.stop();
    const computation = Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        const data: T = refs.reactiveFn(c);
        if (!skipUpdate || !skipUpdate(refs.data, data)) {
          refs.data = data;
          forceUpdate();
        }
      })
    );
    return () => {
      computation.stop();
    };
  }, deps);

  return refs.data as T;
};

All existing tests are passing with this implementation.

[ksinas]

I wouldn't change it to setTimeout. You could just always stop the computation on the start of the useEffect.

That's the same solution I proposed earlier. Will we see it in the next version? Then I can throw away my fork

[CaptainN]

I'll get it in

[CaptainN]

This kind of opens the door for reusing that computation... I need to think about that.

Can you try this out and verify that it's working?

[mfen]

Some light testing shows this working for us, no double subscription or extra changed messages. :+1:

[CaptainN]

@mfen I've committed this to the useFind branch, which currently represents the next release branch (2.4). So this will be in the next release.

Also, I can't reuse the initial computation as it turns out. The two have differing requirements. Not a big deal, but wanted to close that door.

[fumbleforce]

I cloned the latest beta as I have been experiencing the same issue of sub-unsub-sub, and the latest PR does not seem to fix it for me unfortunately.

We originally wrote our code wtih withTracker, and had the issue, so we tried to switch to useTracker, but had the same issue. Going back to the old code and using the ancient 0.2.x version of withTracker, replacing the deprecated hooks with their UNSAFE equivalents, works like a charm, and we have no unnecessary unsubs.

Reproduction here, just the simple initial react starter with a publish. The problem only appears at initial load, reloads after changes does not trigger the issue. https://github.com/jorgeer/react-meteor-data-double-sub

[CaptainN]

An update: This was resolved in the specific reported case by supplying deps to useTracker - however, this issue may expose a real bug that should be investigated with the "depsless" implementation of useTracker.

(The above linked repo does provide a reproduction.)

[CaptainN]

Is this still an issue with the latest version? It may have been addressed in the fixes for React 18

[henriquealbert]

Any updates on this one @CaptainN, or can we close it?

[CaptainN]

I think we are waiting on word from @ksinas ? I think this is fixed though.