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

Double subscriptions when using useTracker in lazy components #324

Open
ksinas opened this issue Mar 2, 2021 · 26 comments
Open

Double subscriptions when using useTracker in lazy components #324

ksinas opened this issue Mar 2, 2021 · 26 comments
Labels
bug in-discussion We are still discussing how to solve or implement it

Comments

@ksinas
Copy link

ksinas commented Mar 2, 2021

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

CaptainN commented Mar 2, 2021

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 CaptainN added the needs-reproduction We can't reproduce so it's blocked label Mar 2, 2021
@CaptainN
Copy link
Contributor

CaptainN commented Mar 2, 2021

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:

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

@CaptainN
Copy link
Contributor

CaptainN commented Mar 2, 2021

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

@ksinas
Copy link
Author

ksinas commented Mar 2, 2021

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

ksinas commented Mar 2, 2021

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

CaptainN commented Mar 2, 2021

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

ksinas commented Mar 2, 2021

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

CaptainN commented Mar 2, 2021

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

CaptainN commented Mar 2, 2021

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

ksinas commented Mar 2, 2021

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

CaptainN commented Mar 2, 2021

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

ksinas commented Mar 3, 2021

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

CaptainN commented May 27, 2021

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

mfen commented Oct 27, 2021

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

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

CaptainN commented Oct 29, 2021

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

ksinas commented Oct 29, 2021

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

I'll get it in

@CaptainN
Copy link
Contributor

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

mfen commented Oct 29, 2021

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

@CaptainN
Copy link
Contributor

@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.

@CaptainN CaptainN added the ready We've decided how to solve or implement it label Oct 30, 2021
@fumbleforce
Copy link

fumbleforce commented Nov 11, 2021

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

CaptainN commented Nov 23, 2021

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 CaptainN added bug and removed needs-reproduction We can't reproduce so it's blocked ready We've decided how to solve or implement it labels Nov 23, 2021
@CaptainN
Copy link
Contributor

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

@henriquealbert
Copy link
Member

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

@CaptainN
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug in-discussion We are still discussing how to solve or implement it
Projects
None yet
Development

No branches or pull requests

6 participants