[Concurrent] Safely disposing uncommitted objects

danielkcz commented 5 years ago

How to safely keep a reference to uncommitted objects and dispose of them on unmount?

For a MobX world, we are trying to prepare for the Concurrent mode. In short, there is a Reaction object being created to track for observables and it is stored within useRef.

The major problem is, that we can't just useEffect to create it in a safe way later. We need it to start tracking the observables on a first render otherwise we might miss some updates and cause inconsistent behavior.

We do have a semi-working solution, basically, a custom made garbage collector based on setTimeout. However, it's unreliable as it can accidentally dispose of Reactions that are actually being used but weren't committed yet.

Would love to hear we are overlooking some obvious solution there.

gaearon commented 5 years ago

@mweststrate

danielkcz commented 5 years ago

No need to ping him, he knows about it.

Either way, I wanted to ask for a more generic solution, not only related to a MobX. I can imagine similar problems will be happening all over the place once people start preparing for Concurrent. It feels like that React is not giving enough tools to tackle the problem gracefully.

gaearon commented 5 years ago

I’m tagging him precisely because “he knows about it” and because we’ve had conversations in the past about it. :-)

Given that he knows constraints of both I thought his input could be valuable.

As for your question — the design tradeoff we’re going for is that we don’t have a cleanup phase for renders that never committed. Rendering isn’t supposed to “allocate” resources that have to be cleaned up later. Instead we rely on the garbage collector. Resources that have to be disposed are only safe to allocate after the first commit.

I don’t know for certain if tradeoffs chosen by MobX are compatible or not with this approach, as I don’t know enough about MobX.

Can you explain what “reactions” are and why GC isn’t sufficient for them? Or why creating them can’t wait until after the first commit?

danielkcz commented 5 years ago

Can you explain what “reactions” are and why GC isn’t sufficient for them? Or why creating them can’t wait until after the first commit?

Well, I don't know exact internals how it works, it's very entangled there, but my assumption is that Reaction kinda attaches itself to observable variables present in the callback function. So as long as those variables exist, the Reaction won't be GCed. Those variables can definitely exist outside the scope of the component, that's kinda point of the MobX.

I think we have managed to figure out some way around it. Custom GC :) Basically, we globally keep the list of created reactions (in WeakMap) along with a time of their creation. If Reaction gets committed, all is fine and it gets removed from the list. A global timer takes care of cleaning up those that weren't committed in a given time.

I am wondering if you have some idea for a good "timeout" period? I mean time from render to commit. For now, we are thinking like 1 second. I assume it should be a long enough period even on a really slow computer. Is it bad idea to rely on that?

danielkcz commented 5 years ago

Or why creating them can’t wait until after the first commit?

@RoystonS wrote a good explanation of that. We could end up in very ugly situations ...

urugator commented 5 years ago

Can you explain what “reactions”

Mobx automatically manages subscriptions based on usage. You define a function (reaction), which can access observable objects. When the function is invoked, the observables accessed inside the function are added as dependencies to the function (reaction). When you modify an observable, the function is automatically invoked again, resubscribing and optionally running a side effect. We wrap render inside such function, so that if you access an observable in render, it creates a subscription:

const disposer = reaction(
  () => {    
    // accesses observables, creating subscriptions
    render()
  }, 
  () => {
    // a side effect to invalidate the component
    setState() // or forceUpdate() 
  }
)

why creating them can’t wait until after the first commit

The subscriptions are determined by rendering logic, so we would have to re-run the rendering logic in effect (double render).

why GC isn’t sufficient for them

While reactions come and go (together with react's component instances), the observable objects possibly stay for the whole app lifetime and they hold references to reactions (preventing them from being GCed).

RoystonS commented 5 years ago

Basically: we're trying to avoid double-rendering all MobX-observer components. The only time we actually need to double-render is if StrictMode/ConcurrentMode began rendering our component but then never got to the commit phase with us, or if something we're observing changed between initial render and commit. Taking a double-render hit on every component to cope with those (hopefully edge) cases is something we'd like to avoid.

mweststrate commented 5 years ago

@gaearon @urugator's description is pretty accurate. Whether it should be fixed through a React life-cycleish thing or by doing some additional administration on MobX side (timer + custom GC) have both it's merits. So let's leave that aside for now, and just focus on the use case:

Basic concepts

But the reason why immediate subscription is important to MobX (as opposed to a lifecycle hook that happens after render, such as useEffect) is because observables can be in 'hot' and 'cold' states (in RxJS terms). Suppose we have:

A collection of todo items
An expression (we call this a computed) that computes the amount of unfinished computed items
A component that renders the amount of unfinished computed items.

Now, the expression computed can be read in two different modes:

If the computed is cold, that is, no one is subscribed to it's result; it evaluates as a normal expression, returning and forgetting its result
If the computed is hot, it will start observing its own dependencies (the todo items), and caching the response as long as the todo item set is unaffected and the output of the computed expression is unchanged.

MobX without concurrent mode

With that I mind, how observer was working so far is as follows:

During the first (and any subsequent) render, MobX creates a so called reaction object which keeps track of all the dependencies being read during the render and subscribes to them. This caused the computed to become hot immediately, and a subscription to the computed to be set up. The computed in turn subscribes to the todos it used (and any other observable used). So a dependency graph forms.
Since there is an immediate subscription, it means that the reaction will no longer be GC-ed as long as the computed exists. Until the subscription is disposed. This is done automatically done during componentWillUnmount. This cause the reaction to release its subscriptions. And in turn, if nobody else depends on the 'computed' value, that will also release its subscriptions to the todos. Reaction objects can now be GC-ed.

So what is the problem?

In concurrent mode, phase 2, disposing the reaction, might no longer happen, as an initial render can happen without matching "unmount" phase. (For just suspense promise throwing this seems avoidable by using finally)

Possible solutions

Solution 1: Having an explicit hook that is called for never committed objects (e.g. useOnCancelledComponent(() => reaction.dispose())

Solution 2: Have global state that records all created reaction objects, and remove reactions belonging to committed components from that collection. On a set interval dispose any non-committed reactions. This is the current (WIP) work around, that requires some magic numbers, such as what is a good grace period? Also needs special handling for reactions that are cleaned up, but actually get committed after the grace period!

Solution 3: Only subscribe after the component has been committed. This roughly looks like;

During the first render, read the computed value cold, meaning it will be untracked
After commit (using useEffect) render another time, this time with proper tracking. This serves two goals:
1. The dependencies can be established
2. Any updates to the computed between first rendering and commit will be picked up. (I can imagine this is a generic problem with subscribing to event emitters as part of useEffect, updates might have been missed?)

The downsides:

Every component will render twice!
The computed will always compute twice, as there is first a 'cold', which won't cache, and then a 'hot' read. Since in complicated applications there might be entire trees of depending computable values be needed, this problem might be worse than it sounds; as the dependencies of a computed value also go 'cold' if the computed value itself goes cold.

Hope that explains the problem succinctly! If not, some pictures will be added :).

gaearon commented 5 years ago

Does MobX have bigger problems in Concurrent Mode? How valuable is it to fix this in isolation without addressing e.g. rebasing?

mweststrate commented 5 years ago

I think the later problem can be largely avoided by making a clear distinction between component own state (use useState instead of MobX) and state that lives outside the component tree (external domain state).

If I am not mistaken, my intuition is that rebasing only makes sense for 'volatile ui state' such as the state of pending requests, switching tabs / pages, charts etc. But state that is domain specific (e.g. a collection of todo's), typically doesn't need rebasing as that would lead to weird behavior for the end-user?

On Wed, Apr 10, 2019 at 4:36 PM Dan Abramov notifications@github.com wrote:

Does MobX have bigger problems in Concurrent Mode? How valuable is it to fix this in isolation without addressing e.g. rebasing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/15317#issuecomment-481716405, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhOMREb3lbrEL4q9sccIXfNj5f5vfks5vffcHgaJpZM4ccUAP .

NE-SmallTown commented 5 years ago

Any picture? Thanks~ :)

radex commented 5 years ago

We have a very similar issue to MobX's with 🍉 WatermelonDB, but even more tricky because of asynchronicity.

When connecting the data source to components, we subscribe to the Observable. This has to happen directly in render, because:

if we subscribe in an effect/on did mount, we'll ALWAYS render twice, and the first render will be wasted, even if we have the data ready to render and we can skip a bunch of unnecessary work.
it wouldn't be appropriate for an useObservable hook to return null on initial render, because it's not always clear what the component developer is supposed to do with it. For example, if an observable value is necessary to fetch another observable value, the developer would have to make a whole bunch of extra logic just to get to the return null (since we can't just bail early and render null with hooks - like we can today with a withObservables HOC).
it would be appropriate for useObservable to throw a Promise and suspend, however, we risk showing stale data (which can lead to bugs or ugly glitches) if we treat the data source like an async promise, not a reactive Observable — it could be that between the time the initial data is fetched, and component re-rendered, the value of the observable has changed again. But it's not possible to know that on the second render if we don't stay subscribed to it. Which means we have to subscribe before we throw a Promise — and have to deal with cleaning up the subscription if the component never ends up being rendered

CaptainN commented 5 years ago

We have a similar situation in Meteor, where we set up a computation and its observers the first time it's run. If we use useEffect we have to either wait for useEffect to do that, causing a render of no content, or use useLayoutEffect without touching the dom, which would violate the suggestions and complicate the API for our users and still render twice if somewhat synchronously, or render twice with useEffect - once without setting up observers which is hacky (or doing a full create/dispose lifecycle) and then always at least one additional time after useEffect (this was janky in testing).

What I ended up doing is something based on setTimeout, but it takes a lot of juggling (we also have some backward compat code which complicates our implementation. It has to set up the computation and a timeout, then in useEffect it has to look for a few things; Is the computation still alive (did setTimeout trigger dispose)? If not rebuild it, and re-render (back to 2 renders). Did we get a reactive change between render and useEffect? If so, re-run the computation, and re-render (this is fine, because it should anyway).

I originally set the timeout to 50ms, assuming commit would happen relatively quickly, but in testing in concurrent mode 50ms missed almost all commits. It often took longer than 500ms to commit. I've now set the timeout to 1000ms. I also saw that there are a lot of discarded renders for every commit, between 2-4 to 1. (I have no idea how to write tests for all this, this is based on some counter/console logging.)

Since there is a lengthy gap between render and commit, I had to decide what to do for reactive changes that happened in renders which will never be committed. I could either run the user's reactive function for every change, knowing it might be run by 4 different handlers without doing anything, or block reactive updates until the component was committed. I chose the latter.

I tried a workaround using useLayoutEffect. I incorrectly assumed it would run in the same event loop as render. If it had, the code for juggling when and if to build/rebuild/re-run/re-render could all be greatly simplified, and a timeout of 0ms or 1ms would work just fine. In Strict Mode, this worked beautifully (and greatly simplified our implementation). In practice, in concurrent mode, useEffect and useLayoutEffect performed no differently from each other.

BTW, one additional thing we found challenging was trying to leverage a deps compare. We had at one point copied some parts of react out of core to manage that, before just switching to useMemo to use its deps compare without it's memoization. I understand that may lead some some additional re-creations, our old HOC used to do that anyway (and it does that now if a user supplies no deps to our useTracker hook), so I went with useMemo just to reduce our overhead - but it'd be nice if the deps compare functions were somehow available to use.

CaptainN commented 5 years ago

A quick additional observation - one of the things I have in place is a check to make sure that if the timeout period elapsed (currently 1 second), it will make sure to restart our computation when/if the render is eventually committed in useEffect. This has almost never triggered outside of concurrent mode (and it rarely triggers inside concurrent mode at 1second). It did trigger today during a particularly janky local dev reload. All of this would be so much simpler if there was a way to reliably clean up after discarded renders without resorting to timeout based GC hacks.

aleclarson commented 5 years ago

Using dual-phased subscriptions (phase 1: collect observed object/key pairs and their current values, phase 2: subscribe to them), you can avoid any timer-based GC and postpone subscribing until React commits. In phase 2, be sure to check for changed values before subscribing, so you can know if an immediate re-render is required. This is what I did with the Auto class in wana. Of course, this only applies to observable objects. Streams and event emitters, not so much.

CaptainN commented 5 years ago

We'd have to modify a bunch of stuff in Meteor core to convert that to a dual-phase subscription model like that (although we can fake it by just setting up the subscription, running the computation, then disposing of it immediately, to restart it again in useEffect). An additional problem is that the return data from a Meteor computation is not immutable, and can be any type of data (with possibly very deep structures), so a deep compare would be necessary to make sure data hasn't changed, or we'd have to render twice (same problem as before).

aleclarson commented 5 years ago

We'd have to modify a bunch of stuff in Meteor core to convert that to a dual-phase subscription model like that

The issue with Meteor is that its tracking system doesn't associate "observable dependencies" with state. In other words, the Tracker.Dependency instance is disconnected from what it represents. Although, you could get around that by updating a nonce property on the dependency whenever it emits a change event.

(although we can fake it by just setting up the subscription, running the computation, then disposing of it immediately, to restart it again in useEffect)

Wouldn't you miss changes between render and commit if you did it that way?

An additional problem is that the return data from a Meteor computation is not immutable, and can be any type of data (with possibly very deep structures), so a deep compare would be necessary to make sure data hasn't changed, or we'd have to render twice (same problem as before).

Yep, that's the problem with not using observable proxies that track every property access.

aleclarson commented 5 years ago

Another good use case for useCancelledEffect is being able to lazily allocate shared state (which is stored in a global cache or in context) during render, without the risk of leaks or the need to implement a timer-based GC.

CaptainN commented 5 years ago

Wouldn't you miss changes between render and commit if you did it that way?

Yes. It's actually that way now even with the timer. If an update comes in before the render is committed (which takes about a half a second in my testing), I had to make a choice - run the user's reactive function for every uncommitted computation, or don't run any of them until the render is committed (I detect if an update happened, but don't run user code until later) - I chose the latter. In the example I gave in the other comment, I'm saying we could simply always rerun the computation and user function again in useEffect - but then we have to always eat a second render, and all the rest.

Another good use case for useCancelledEffect is being able to lazily allocate shared state (which is stored in a global cache or in context) during render, without the risk of leaks or the need to implement a timer-based GC.

I had thought it would be useful if we could get some kind of deterministic ID for whatever the current leaf is, which is consistent for all renders of current component, even those that would not be committed. Then I could use my own global store. I think someone explained that couldn't happen though, for various reasons. (Can react-cache be used for this? I'm not clear on what that is useful for.) I could offload the naming/ID responsibility to the user, but that seem onerous, and I'm not sure how I'd be able to detect collisions - though, would I have to? 🤔

aleclarson commented 5 years ago

This problem also exists without Concurrent mode enabled, because no-op state changes can trigger a render that bails out before effects are ever called. Source

kitten commented 4 years ago

We've had to find a solution to this for urql. It's a similar problem to what MobX has in that we're using streams that start a GraphQL operation when subscribed to which then yields either synchronous or asynchronous results (cached or fetching).

The solution to this can be found here: https://github.com/kitten/react-wonka/blob/master/src/index.ts We're essentially dealing with 1) synchronous subscriptions since they may immediately return a result, so they can't be started in useEffect only, 2) cleanups on interrupted or double renders, 3) updates to values in useEffect.

I'll leave some simplified / generic code here in case someone else has to implement it from scratch, since we haven't found a generic solution yet that could be shared (like an alternative to use-subscription for instance)

Interruptible Subscriptions implementation walkthrough

On mount we start the subscription if it hasn't started yet: _(note: most of this is pseudo code since the full implementation is linked above)_ ```js const useObservable = observable => { const subscription = useRef({ value: init, onValue: value => { subscription.current.value = value; }, teardown: null, task: null, }); if (subscription.current.teardown === null) { // Suppose we have a subscription that needs to be started somehow, with a teardown: subscription.current.teardown = observable.subscribe(value => { subscription.current.onValue(value) }); } return subscription.value; }; ``` Then we schedule an immedite teardown/cancellation of the subscription using `scheduler` (this way we get really predictable and cooperative timing): ```js import { unstable_scheduleCallback, unstable_cancelCallback, unstable_getCurrentPriorityLevel, } from 'scheduler'; const useObservable = observable => { // ... if (subscription.current.teardown === null) { subscription.current.teardown = observable.subscribe(value => { subscription.current.onValue(value) }); subscription.task = unstable_scheduleCallback( unstable_getCurrentPriorityLevel(), subscription.teardown ); } return subscription.value; }; ``` On interrupted rendering we won't have any effects run (`useEffect` and `useLayoutEffect`) but on a normal render we don't want to cancel the subscription by calling the `teardown`. We can use `useLayoutEffect` to cancel the teardown (`unstable_cancelCallback`) since that has synchronous timing (after mount during the commit phase) ```js const useObservable = observable => { // ... if (subscription.current.teardown === null) { subscription.current.teardown = observable.subscribe(value => { subscription.current.onValue(value) }); subscription.task = unstable_scheduleCallback( unstable_getCurrentPriorityLevel(), subscription.teardown ); } useLayoutEffect(() => { // Cancel the scheduled teardown if (subscription.current.task !== null) { unstable_cancelCallback(subscription.current.task); } // We also add the teardown for unmounting return () => { if (subscription.current.teardown !== null) { subscription.current.teardown(); } }; }, []); return subscription.value; }; ``` Lastly we'd like a normal `useEffect` to take over for normal cooperative effect scheduling. In the real implementation we also send updates back to a subject which affects the observable, so we do that there as well. For simplification/generalisation purposes that's excluded here though. ```js const useObservable = observable => { // ... // We add an updater that causes React to rerender // There's of course different ways to do this const [, setValue] = useReducer((x, value)) => { subscription.current.value = value; return x + 1; }, 0); if (subscription.current.teardown === null) { // ... } useLayoutEffect(() => { // ... }, []); useEffect(() => { // In useEffect we "adopt" the subscription by swapping out the // onValue callback with the setValue call from useReducer above subscription.current.onValue = setValue; // If we've cancelled the subscription due to a very long interrupt, // we restart it here, which may give us a synchronous update again, // ensuring that we don't miss any changes to our value if (subscription.current.teardown === null) { subscription.teardown = observable.subscribe(value => { subscription.onValue(value) }); } // We'd typically also change dependencies and update a Subject, // but that's missing in this example }, []); return subscription.value; }; ``` Lastly we need to update this to work on server-side rendering. For that we swap out `useLayoutEffect` with a `useIsomorphicEffect` ```js const isServerSide = typeof window === 'undefined'; const useIsomorphicEffect = !isServerSide ? useLayoutEffect : useEffect; ``` And add some code that immediately cancels the subscription on the server-side: ```js const useObservable = observable => { // ... if (subscription.current.teardown === null) { subscription.teardown = observable.subscribe(value => { subscription.onValue(value) }); if (isServerSide) { // On SSR we immediately cancel the subscription so we're only using // synchronous initial values from it subscription.current.teardown(); } else { subscription.current.task = unstable_scheduleCallback( unstable_getCurrentPriorityLevel(), subscription.current.teardown ); } } // ... }; ``` Afterwards we added some code that handles our updates to an input value.

tl;dr We start the subscription on mount synchronously, schedule the teardown using scheduler, cancel the scheduled teardown in useLayoutEffect (since that tells us that effects will run and we're not in an interrupted render), let a useEffect adopt the subscription and switch an onValue updater over to a useReducer update.

If note is that this only works as well as it does for us because we're already handling starting operations only once if necessary and avoiding duplicates, so in a lot of cases our subscriptions are already idempotent. I can totally see that this isn't the case for everyone, so a lot of subscriptions with heavy computations won't be as "safe" to run twice in rapid succession.

This is a little inconvenient to say the least. I'd love if either use-subscription or a built-in primitive could introduce a concept of observing changing values that starts a subscription synchronously but also ensures it's cleaned up properly on interrupts or unmounts. 🤔

mweststrate commented 4 years ago

@FredyC At a quick glance, the above might be cleaner than the current approach in lite 2. Thoughts? (Probably better to discuss further in the repo, just wanted to cc you in!)

dai-shi commented 4 years ago

It's really interesting.

we haven't found a generic solution yet that could be shared (like an alternative to use-subscription for instance)

Let me give a quick trial in TS:

import { useState, useRef, useLayoutEffect } from 'react';
import {
  unstable_scheduleCallback as scheduleCallback,
  unstable_cancelCallback as cancelCallback,
  unstable_getCurrentPriorityLevel as getCurrentPriorityLevel,
} from 'scheduler';

type Subscription<Value> = {
  currentValue: Value; // synchronous value
  unsubscribe: () => void;
};
type Subscribe<Value> = (callback: (nextValue: Value) => void) => Subscription<Value>;

export const useSubscription = <Value>(subscribe: Subscribe<Value extends undefined ? never : Value>): Value => {
  if (subscribe !== useRef(subscribe).current) {
    throw new Error('no support for changing `subscribe` in this PoC');
  }

  const [value, setValue] = useState<Value | undefined>();

  const subscription = useRef<Subscription<Value>>();
  const cleanup = useRef<ReturnType<typeof scheduleCallback>>();
  if (!subscription.current) {
    subscription.current = subscribe(setValue);
    cleanup.current = scheduleCallback(getCurrentPriorityLevel(), subscription.current.unsubscribe);
  }

  useLayoutEffect(() => {
    if (cleanup.current) cancelCallback(cleanup.current);
    return subscription.current?.unsubscribe;
  }, []);

  return value !== undefined ? value : subscription.current.currentValue;
};

CaptainN commented 4 years ago

@dai-shi How does that scheduleCallback function work? Does it use a timeout?

dai-shi commented 4 years ago

@CaptainN I re-organized what @kitten described in a general way. I haven't tested it (at least yet). Having said that, my understanding is that 1) scheduleCallback registers a callback into a queue, 2) useLayoutEffect runs in a higher priority (or the same but somehow before it?), 3) thus cancels the registered callback in the queue before executing.

setTimeout would be used in scheduler, but not like in a naive way.

@kitten @JoviDeCroock Did you figure out which priorities are used in render and layout effect? NormalPriority for the former and ImmediatePriority for the latter, I presume, but not confirmed.

(edit) On second thought, I'm a bit suspicious about this. Suppose, if one of render is really slow (like taking several seconds), React will likely to throw away that render result on updates, but it may(?) keep other components that are not committed yet and will commit them after a long time.

CaptainN commented 4 years ago

@dai-shi That's why I asked it was timeout based. In the Meteor hook we implemented, we just use a timeout. I found that a full 1000ms was needed in order for the subscription (computation in our case) to be preserved in some large percentage of cases (at least on my machine). We had to do a couple of other things though - we also will destroy the computation if an update comes in before the render is committed. And we also check in useEffect if we need to rebuild the computation.

There's a lot of overhead to implement this correctly, all of it is difficult, and there are dozens of bad examples out on the net masquerading as good suggestions, most of which will leak memory and resources, mostly silently. There really needs to be an easier properly supported solution to this common problem.

danielkcz commented 4 years ago

I wonder is the take on this from the React team. Apparently, the use cases are pilling up and with Concurrent more getting more attention it would be silly to expect everyone implementing some hacky workarounds.

Just a wild idea, but what about Suspense? Can we, in theory, suspend rendering until any sort of "rendering setup" is ready? I am not really sure what would be the implications. I know just basics about the whole thing. Would it harm the performance?

@sebmarkbage Can you possibly give some thoughts here? Is this something the React simply won't support officially?

CaptainN commented 4 years ago

@FredyC Suspense is similarly unintuitive. It doesn't "suspend" execution, and then later commit. After you throw a promise, later on a fresh render runs again from scratch. But that doesn't give us the info we need anyway. Suspense is great for when our resource is not yet ready. But if our resource is ready (and it almost always is in Meteor) during the first render, concurrent mode will run it however many times (up to 4 in my testing), and then only commit one of them. But there's no reliable or easy way to determine which ones are tossed.

It'd be great if there were some sort of way to know that all /n/ renders are candidates for the same commit - that'd be ideal. We could run the effect once for the pool, and associate it with the committed render, but that's probably too much overhead or something to set up.

If only there was a "componentWillMount" hook. 🤔

dai-shi commented 4 years ago

While waiting for the official answer, which I think is "won't support", here's another suggestion. Coming from the basics, we make render function side effect free.

const useSubscription = (subscribe, initialValue) => {
  const [value, setValue] = useState(initialValue);
  useLayoutEffect(() => {
    const { currentValue, unsubscribe } = subscribe(setValue);
    setValue(currentValue);
    return unsubscribe;
  }, [subscribe]);
  return value;
};

One caveat as you see is that developers who use this hook has to provide initialValue. But, end users won't see the initialValue rendered because it's the layout effect.

I'm pretty sure many of us already tried such an approach, but just an option to re-consider as it's the closest one to componentWillMount.

Yet another approach would be to store the current value in a global cache.

const cache = new WeakMap();

const useSubscription = (subscribe) => {
  if (!cache.has(subscribe)) {
    const { currentValue, unsubscribe } = subscribe(() => null /* noop */);
    cache.set(subscribe, currentValue);
    unsubscribe();
  }
  const [value, setValue] = useState(() => cache.get(subscribe));
  useEffect(() => {
    const { currentValue, unsubscribe } = subscribe(setValue);
    cache.set(subscribe, currentValue);
    setValue(currentValue);
    return unsubscribe;
  }, [subscribe]);
  return value;
};

This approach can be done with the official use-subscription, which might be appropriate. Obviously, this pattern is not very nice, if we often need customized subscribe functions, like one-to-one to components.

danielkcz commented 4 years ago

The useLayoutEffect even though it runs almost immediately, it's still runs after the first render happens. For MobX purposes, it's useless as we need to watch over that first render.

The global cache might be almost good only if there would be some reliable identity of the component instance. Especially the identity that would be the same over the Concurrent mode retries. We cannot expect users to pass that identity there. Generating something out of thin air and storing with useRef or useState is useless too as we end up with exactly the same problem of having multiple reactions and we won't know which one to dispose of.

dai-shi commented 4 years ago

@FredyC For the MobX case, it would be ideal to work with dual-phased subscriptions. It would be nice if MobX provides such capabilities. As I look at the mobx code, unfortunately not. (Now, I understand where your concerns are coming from.)

if there would be some reliable identity component instance

Totally right about that. I forgot that useCallback is also reset.

kitten commented 4 years ago

@dai-shi I mean, to get this out of the way, this issue is explicitly not about "dual-phased subscriptions" (let's just say subscriptions via/in useEffect to keep things simple), so I do think we all know that that does work, but we'd like our subscriptions to be represented as values in React 1:1 and not eventually match up 😅

sebmarkbage commented 4 years ago

Re: connecting to an external store.

There are a lot of implementation details of current systems. That might make upgrading hard but that doesn't say whether it's a good idea longer term. I think ultimately this problem bottoms out in reading a mutable value in a newly created component (already mounted components can use snapshots in their state). Depending on if your system also wants to support Suspense this might get conflated with Suspense, see below.

We're working on a new hook for mutable external values that deopts in scenarios where it would produce an inconsistent tree. By doing a full batched render in synchronous mode. It's "safe" and it does allow a tree to commit as one. It's unsatisfactory because it will deopt and produce unfortunate perf characteristics in certain scenarios.

It's related to this thread, because it aims to solve scenarios that I'm not sure has been considered yet. I believe that even if we had a clean up hook, you would still leave unsafe scenarios on the table or at the very least deopt in more cases.

In terms of a mutable store, the subscription fills the purpose of invalidating the render if it changes before commit. Otherwise the useSubscription pattern works. However, you also have to think about whether invalidating the render should also take any higher pri updates with it. It's not just about consistency but whether you merge multiple timelines. Additionally, if you always invalidate a render for changes to the store you risk starving out. I think what you really want is to commit the work you've already done even if it's now stale, as long as that commit is consistent with itself.

That means we should continue rendering until we find a case that is impossible to make consistent and only deopt once we hit that. Because if you use it sparingly you don't have to deopt at all. However, you don't need a subscription to accomplish this because it's not the already rendered component that determines whether the tree is inconsistent, it's the next rendered component that we haven't gotten to yet.

Re: suspense.

I agree that suspense intuitively feels like you should be able to open connections during render. That's what our original demos showed. It seems right. However, there are a few caveats.

One is that even when React does its best to preserve already started work, it's surprisingly easy for rebases, especially long lived ones, to invalidate it. Therefore I believe you still need something like a timeout based cache to solve this regardless of what React does.

Additionally, there are other problems with this approach with regard to when invalidation of data that has already rendered should happen. The solution to that is keeping an object in state that represents the current lifetime like we showed at the latest React Conf. If you use that approach then you also solve the problem of when to invalidate opened requests. When you explicitly cancel the lifetime of the subtree, then you cancel any requests opened by that subtree.

Now, in theory we could do a traversal for most cases but it wouldn't be fool-proof and it would make React slower for everyone. To be convinced of this, we would have to see that the whole system overall that spawns from this actually works well, not just in theory, and that its convenience is worth making React slightly slower for everyone.

danielkcz commented 4 years ago

I am probably misunderstanding something, but having a hook to deopt rendering won't work well for MobX case. It would mean to slow down most of the components that work with observables on purpose, right? Or to be precise it would be same performance as without Concurrent mode. That sounds more like a foot gun and not a solution.

With the current solution based on timers, it's ugly, but there is only a risk memory leaks which is not that bad in comparison to deoptimizing rendering on purpose.

@sebmarkbage By any chance, the idea of having hook that provides unique component instance identity does not sound useful? It should be the same over the rendering attempts. Ideally some sort of object so it can be utilized in WeakMap and we will be able to keep info about things that should happen only once.

dai-shi commented 4 years ago

I mean, to get this out of the way, this issue is explicitly not about "dual-phased subscriptions"...

Please forgive me, @kitten. I was wrong. I just stumbled upon this issue while I was developing a new library.

I may misunderstand something either for suspense support. Still figuring out best practices.

Just found https://github.com/FormidableLabs/urql/pull/514, so you could fix it in a better way in urql. That's nice.

sebmarkbage commented 4 years ago

@FredyC The issue with the subscription based approach is what you do when the subscription fires after we’ve rendered that component but before we’ve committed it onto the screen and we get an update. React can do two things with that information. If React doesn’t immediately stop then the rest of the tree will get the new state and it will be inconsistent and cause tearing problems. If React does restart, it’s worse than the deopt approach because you eagerly throw away work even in cases where you wouldn’t need to. If you restart but keep it non-synchronous rendering you risk keep throwing away the work as you get new updates and you starve which is really bad.

So the subscription solution is actually a much worse deopt or it has the tearing problems.

Or to be precise it would be same performance as without Concurrent mode. That sounds more like a foot gun and not a solution.

Yes, this is indeed a foot gun but there’s not much we can do about that. A mutable state that mutates and then later dispatches a change notification that the state has already changed inherently has this property.

It’s a known property in systems that use threads too. Immutable data lets you about snapshots and allows work to be queued and parallelized. Mutable data requires locks and synchronization.

It comes with some downsides too and it’s a tradeoff. We can’t magically fix that.

The deopt approach is a decent compromise. At least it’s not really worse than synchronous React. It does make the story more complicated because you can’t really just say to use the same state for everything since that risks deopting too much. So that makes it a foot gun. It’s not React creating this tradeoff though. It’s just the realities of the tradeoffs of using mutable state.

kitten commented 4 years ago

@dai-shi Yes, but our solution is only possible because we can subscribe on mount and unsubscribe immediately which is our implementation for getCurrentValue, so we’re side-stepping the problem in our new approach.

We essentially stop listening between the first render (mount) and the commit phase. So this only works in our case because we can stop all side-effects in-between both phases.

@sebmarkbage Are the docs in use-subscription still correct about potential deopts there? As far as I understand that hook it doesn't cause any side-effects during render that trigger an update, but only in useEffect and in a listener callback. The comment in the issue seems to highlight the case where getCurrentValue inside useEffect may trigger a state update immediately. Is that what can also cause a deopt?

markerikson commented 4 years ago

@sebmarkbage : can we get a definition of what "deopt" specifically means in this context? The React team has thrown that phrase around a lot, and it would help to know exactly what you're referring to.

Is it simply "a synchronous re-render during the commit phase", or something more specialized than that?

kitten commented 4 years ago

As @dai-shi pointed out in the other issue, this gist by @bvaughn clears things up in terms of what “deopt” means, when it happens, and what may cause tearing https://gist.github.com/bvaughn/054b82781bec875345bd85a5b1344698

If I’m not mistaken, “deopt” means that all updates triggered inside useSubscription‘s subscription or listener in the same tick will be batched and all changes will be committed and flushed immediately altogether. So for certain use cases, this is probably desirable actually, but may be expensive, because I don’t think this will update will be sliced, since the point of this big batch is to avoid tearing.

Tearing (which I’m more unsure of) may occur with useSubscription (and I suppose similar approaches) when either before this big commit phase or during it the subscription updates again. In such cases the newly mounted consumers of a subscription may temporarily be “more up-to-date” and additional update cycle will start.

This doesn’t seem to be an issue with useMutableSubscription, however this hook may trigger an interrupt by throwing and will restart the entire (then even larger) batch

sebmarkbage commented 4 years ago

@markerikson It can mean slightly different things in different implementation details. For example, it might not need to be in the "commit" phase. We can do it before the commit phase if we know it's going to happen earlier. It always means switching to synchronous rendering though.

It may also mean including more things in the batch than is necessary. E.g. normally React concurrent can work with multiple different pending transitions independently but deopt might group them into one batch.

For example if you have pending transitions loading it might also need to force those to be included into one big batch and force them to show early fallbacks. Such as pending startTransition. This can be an unfortunate experience if forcing fallbacks means hiding existing content while loading but you really just wanted to show a temporary busy spinner.

Another case we haven't built yet but that I'm thinking of is enter/exit animations. It might force the animation to terminate early.

I.e. new features that don't work with synchronous rendering can become disabled if we're forced into synchronous rendering. I think that's the worst part of the deopt. Not actually the rendering performance.

The cases when deopts happen is also an interesting discussion point that varies by implementation details. E.g. the useMutableSource approach is meant to trigger the deopt less often.

CaptainN commented 4 years ago

Being able to deopt would be great for Meteor's legacy container and what should become the legacy hook, which can do a bunch of unknowable things in its general tracker. It will always create a computation, but might also fetch data, and or set up subscriptions. If there is a way to opt out of concurrency (deopting), that'd actually be ideal. We could create a separate set of specialized hooks which follow the rules in more specialized hooks to gain the purity benefits (which we probably should have anyway). I'm also looking at a new clean implementation of a general hook to support Suspense, and follow the rules of hooks.

dai-shi commented 4 years ago

While developing another library, I encountered this issue again. (All cases for me are when using libraries that I don't have control. Yeah, just like all of us here.)

As I understand the current best workaround is to use timers, I made a small hook for it. There's no deep thought, so it may contain a potential bug. https://github.com/dai-shi/gqless-hook/blob/d2ee1778cbf3236932b219f332be877b2a5cfd3a/src/useCleanup.ts#L8-L26

export const useCleanup = () => {
  type Cleanup = {
    timer: NodeJS.Timeout;
    func: () => void;
  };
  const cleanups = useRef<Cleanup[]>([]);
  const addCleanup = useCallback((func: () => void) => {
    const timer = setTimeout(func, 5 * 1000);
    cleanups.current.push({ timer, func });
  }, []);
  useEffect(() => {
    const cleanupsCurrent = cleanups.current;
    cleanupsCurrent.forEach(({ timer }) => clearTimeout(timer));
    return () => {
      cleanupsCurrent.forEach(({ func }) => func());
    };
  }, []);
  return addCleanup;
};

Does it look OK?

trusktr commented 4 years ago

Without cleanup primitives for all cases (if part of a component runs, a final cleanup is a must) then this becomes needlessly complex for libraries built on top of React.

Any libs relying on timeouts, dual phase setup, or whatever, are now relying on React's internal implementation, which makes React less able to migrate to better implementation later without breaking libs.

All APIs (whether React, or not) should have creation APIs with reciprocal disposal APIs for all cases. If everyone follows this principal for an API, then all libraries built on top of such API will always have a reliable way to clean up.

In React's case,

render has the potential to allocate memory (especially when using third-party state tracking libs of any sort, no need to understand how they actually work).
thus render should always have a guaranteed reciprocal cleanup method (f.e. useCancelledEffect or something like described above, or see idea below) where for every batch of render calls (or even just a single render call) there is always guaranteed to be a cleanup call (f.e. if a component renders at least once, then a cleanup call must always happen after no more render calls will ever happen).

Following these basic principals are the basis of good software design, whether that's in React or anything else.

Another idea is maybe update useEffect instead supply a cleanup function as a second arg instead of having an effect return it:

useEffect(
  () => console.log('instantiate some stuff'),
  () => console.log('destroy some stuff'),
)

then people interested in writing libs that depend on render use it like this:

useEffect(
  () => /*empty, this never runs if render is not committed*/,
  () => console.log('destroy some stuff from that first render'),
)

where useCancelEffect is effect the same thing but without requiring a first arg.

Main point: there should be something that can signal to cleanup if any part (at least any reactive part) of the component has ran.

markerikson commented 4 years ago

@trusktr : React does have a place to do cleanups: in the effect hooks, which will always run after a committed render.

The entire point of CM is that allocating values that have side effects, while rendering, is not safe because that render might not get committed.

I understand that you're asking for something else beyond that, but people seem to be dancing around the issue that allocating things with side effects while rendering has never been correct. Well, now it matters.

trusktr commented 4 years ago

Seems like the React Hooks effort is battling against the JavaScript language.

State that will be used in a React component can technically be accessed from anywhere, for example an object with a property that is backed by an accessor that tracks state. And now React team has to explain to users why they shouldn't write code (or use any such library) that uses such language features for dynamic stuff.

Adding a simple hook (f.e. onCancelled) would be fairly simple for React to implement (I think) and it would prevent a lot of complicated code in code written on top of React (namely those state tracking libs). It's a win-win.

Now react basically has poor man's classes (React.FC, and worse than classes because now every "method" is re-created on every render), which is effectively the same as instantiating a class that extends React.Component, calling its render method, and never calling componentWillUnmount, which doesn't seem like a good idea.

In the code I write (in general, not talking about React) I always try to write explicit cleanup code that is always guaranteed to run after any object (or poor man's class scope) has been instantiated and when that object (or scope) is no longer needed.

This is a good practice that naturally leads to code that is easier to manage in the long run and in new and unexpected scenarios that may arise, and at worse a practice that can minimize the size of memory leaks (leaks are too easy to create, especially if never thinking about explicit cleanup, which is a far too common thing for novices to not think about).

trusktr commented 4 years ago

Besides all of that, isn't an uncommitted render one render too many? Maybe React, basically using poor man's classes, may as well have a useRender and the return type of React.FC should always be void.

markerikson commented 4 years ago

@trusktr No idea what you're trying to say with that last bit.

Here's the canonical reason why a render might get thrown away:

React starts a low-priority render pass (say, due to an AJAX response coming in)
React pauses partway through the render pass due to frame timing limits. It's called render() for half of the components in the tree so far.
During the pause, the user presses a key in an input
This starts a high-priority render pass. React executes that render pass immediately and commits it
React now returns to the low-priority render pass that was interrupted. The input potentially changed a lot about what should be rendered right now - maybe some components got unmounted or something.
Therefore, some or all of the WIP component render output gets thrown away as React recalculates what the new UI should look like by applying the "received AJAX data" state change on top of the "input update" committed component tree.

So, where in that process is React supposed to do additional cleanup work, when it has no idea what side effects happened in your code, and those renders were never committed?

The question here shouldn't be "how can I clean up work that got thrown away?" It should be "how do I do my work once I know that the component has actually mounted?"

If there are truly use cases that aren't solvable with the current primitives that React provides, then the community should collect a list of specific use cases that are completely incapable of being solved so that we can figure out what primitives are needed.

But so far most of what I seem to be seeing in all these different discussions is "I've been breaking the rules and now that's going to cause me problems, how can I keep breaking the rules without it causing me problems?".

gaearon commented 4 years ago

We’re focused on the “interruption” case but that’s not the only one where this matters. Another example you can think of is a hypothetical gesture API that generates keyframe animations between multiple states (e.g. an image gallery that you can scrub through with your finger). In this case we’d want to do several render passes to precalculate the keyframes, but none of them get committed until you lift the finger. So this notion of “cleanup” becomes really confusing because the thing you are “cleaning up” was never committed to the screen in the first place. It doesn’t have any particular lifetime.

The point we’re trying to make is that rendering is considered a process that should not allocate resources beyond what a traditional GC would collect. Because even if we built a user-space collector for it, it would have poorer performance characteristics and undermine the ability to do these renders fast. Since now we can’t just throw away trees. This undermines all the features that we’re building on top of it.

That said, you could just rely on GC eventually. The WeakRef proposal lets you add some cleanup logic. Its timing would not be deterministic, but neither would it be for a similar React-managed mechanism. So this seems like the longer term solution for the rare cases where you can’t use an effect and for some reason have to allocate a resource during render.

trusktr commented 4 years ago

I think of the canonical reason why a render might get thrown away as the following:

because there's no way to prevent a React.FC from returning something, so basically they always "render" because they always construct a vdom tree. Otherwise, with an idea like useRender in my last comment, React could selectively skip renders for any of the reasons you listed (saving CPU and memory cost, and avoiding the above problem in a different way).

So, where in that process is React supposed to do additional cleanup work, when it has no idea what side effects happened in your code, and those renders were never committed?

Why should any library like React ever need to know about side effects in my code? It should never need to know. It should only provide a guaranteed cleanup method (after all, it's (poor man's) classes at this point).

"how do I do my work once I know that the component has actually mounted?"

When using only the React APIs, then that question is already answered with useEffect. But as mentioned above, people are making other libraries that are activated by a React.FC being called and there being cases when the FC is called and no cleanup opportunity is presented.

This problem is the same as a class constructor being ran and instantiating third-party state tracking libs, but no reciprocal cleanup method ever being called and thus those libs tracking state forever. That's exactly what the body of a React.FC is: the constructor of a poor man's class, and React not every calling a cleanup "method" at any point in the future if that component has not been "committed".

Imagine writing all C++ classes without destructors. That's the same issue.

gaearon commented 4 years ago

But as mentioned above, people are making other libraries that are activated by a React.FC being called and there being cases when the FC is called and no cleanup opportunity is presented.

We’re going in circles but let me reply to this. People can write any kinds of libraries — we can’t stop them. But the React component contract has always emphasized that the render method is meant to be a pure function of props and state. This makes it throwaway-friendly. It is not intended as a constructor, in fact it’s the other way around — it’s React classes that act as poor approximations of functions. Which is why React is converging on a stateful function API now.

If a library chooses to ignore the rules and mutate a list of subscriptions during render, it is its business. But it shouldn’t be surprising that when we add new higher-level features to React that rely on the component contract having a pure render method, those features may not work. I think it’s fair to say that render being pure is well-documented and one of the very first things people learn about React.

facebook / react