Computed values should keepAlive by default

[bjb568]

Intended outcome: If I create a computed value and access it twice, it should only compute its value once (even if there is no reaction that depends on its value). This is the behavior of other state management libraries and of computed values in the signals standard proposal, so new users would likely expect this of MobX.

Actual outcome: Untracked computed values recompute the value each time the value is accessed.

How to reproduce the issue:

const computedValue = computed(() => console.log('computed'));
setInterval(() => computedValue.get(), 100);
// Should only log once

Passing { keepAlive: true } fixes the issue, but shouldn't that be the default behavior? With #1799 I don't see any downsides to having all computed values be keepAlive.

Note that this would likely be a semver major change.

Versions
6.13.7

[Tzahile]

I opened a PR addressing this issue.
I use mobx for more than 3 years but only recently discovered this behaviour 🥲
Hope it will be useful for many

[js2me]

@bjb568 @Tzahile is that possible to pass keepAlive: true globally for all computed ?

[js2me]

I think better to add this into global configuration because this is major change?

[Tzahile]

I've just added a changeset, and it is indeed considered as a MAJOR (and not minor as @bjb568 suggests - since users might want to change their code accordingly, i.e when they want explicitly a computed to be suspended)

[Tzahile]

We have 2 options, depending on @bjb568’s decision:

1. Add a global config option to control the default keepAlive value for computeds

   • ship this in the current release (minor).
   • flip the default behaviour in the next major release.

2. flip the default behaviour now, but also add a temporary global config escape hatch.

   • ship this in the current release (minor).
   • set the global flag to false to preserve current behaviour unless users opt in.
   • add a warning that 'this global flag will be removed in the next major release'.

in both cases, maybe indeed we should add a PR with the flag so that we both can use this already, and have easier adoption when next major is released

[bjb568]

@Tzahile Option 2 makes sense to me. I think it's helpful to have a global config to make it easier to upgrade but the global config probably doesn't need to be kept long term.

[Tzahile]

done
I've added a global flag with value 'false'.

@bjb568 I think we can actually leave this flag in next major instead of deleting it, so that users who wish to preserve 'old' behaviour and DONT want to go to each computed and set keepAlive explicitly, will be able to do so by setting the global flag to false. wdyt?

[bjb568]

@Tzahile Yes, keeping the flag for one more major version seems reasonable to me.

[urugator]

It's not default, because keepAlive computed holds references to it's dependencies and to the returned value indefinitely, therefore they can't be garbage collected, potentially causing memory leaks. This is unrelated to #1799, which only makes sure it doesn't recompute eagerly on dependency change while nobody is watching.

[bjb568]

@urugator From my testing I wasn't able to find a memory leak.

If using a computed with keepAlive leaked memory, I would expect the following code to increase its heap usage without bound. However, when running it, after 10 seconds (once "Clearing interval" starts to be printed) it looks like it is garbage collecting the old ArrayBuffers that were held in the observable.box.

setInterval(() => {
  const megabytes = performance.memory.usedJSHeapSize / 1e6;
  console.log(`Creating new buffer and computed. ${megabytes.toFixed(2)} MB used.`);

  // Allocate 100 MB of memory
  const observable = observable.box(new ArrayBuffer(100 * 1e6));
  const computedValue = computed(() => observable.get(), { keepAlive: true });

  // For the next 10 seconds, keep accessing the computed value and prevent GC
  const interval = setInterval(() => {
    computedValue.get();
  }, 200);

  setTimeout(() => {
    console.log("Clearing interval");
    clearInterval(interval);
  }, 10000);
}, 1000);

[urugator]

In your example observable and computed share the same scope. If you replace computed with autorun it won't leak either, but that shouldn't lead to conclusion that autoruns don't have to be disposed.

[mweststrate]

As @urugator is describing, the behavior is indeed working as intended. There is a bi-directional pionters between observable values <-> computed values <-> reactions. The mental model here is computed values only need to be cached if anyone is actively observing them, and if not, they should not hold on to memory so that things can be freed.

If there is no one observing a value over time, there is no need to cache either, and the idiomatic way to observe over time is to set up a reaction (or some derivative of it). KeepAlive can be forced in the current API on a per instance basis, but we don't support it out of the box as it will lead to unexpected memory leaks.