Actions in computed can cause computed to stall silently

[upsuper]

Recently we had an incident that some part of the UI stops working with no error thrown. Digging into it, it seems that one computed just stops updating regardless of its dependencies still being changing.

The internal state shows that computed has dependenciesState_ being STALE, while all its observers have that being UP_TO_DATE. If I make its observer observe something else and trigger that thing, this computed gets back to a normal state and starts updating again.

After further inspection, I believe this is caused by an action within a computed deep in the dependency chain. That action marks the computed POSSIBLY_STALE, but later the evaluation process marks all the observers UP_TO_DATE. The computed being POSSIBLY_STALE means no changes upstream would be propagated beyond the computed, and thus no observers would be triggered by the computed, and the computed gets stalled.

This is a simplified reproduction of the issue:

const counter = mobx.observable.box(0);
const atom = mobx.createAtom('');
const computed = mobx.computed(() =>  {
  atom.reportObserved();
  mobx.runInAction(() => atom.reportChanged());
  return counter.get();
}, { equals: (a, b) => (Math.floor(a / 2) === Math.floor(b / 2)) });

export const App = observer(() => (
  <button onClick={mobx.action(() => counter.set(counter.get() + 1))}>
    Inc {computed.get()}
  </button>
));

The computed ought to output a result every second click to the button, but it never gets updated because it is trapped into the weird state mentioned above, and there is no error thrown for this.

I fully understand that computed is not supposed to have side effect like this, and I totally agree with it and condemn that code. But in a sizable codebase and with a large development team, it is not always easy to capture such instances and enforce the rules without something more explicit. Having it fail silently also makes it very hard to diagnose.

Should we consider maybe adding at least some DEV checks to detect such issue?

[mweststrate]

Computed values must be pure but your example has a side effect (I think that runInAction will print a warning?)

…

On Thu, Apr 10, 2025, 00:25 Xidorn Quan ***@***.***> wrote: Recently we had an incident that some part of the UI stops working with no error thrown. Digging into it, it seems that one computed just stops updating regardless of its dependencies still being changing. The internal state shows that computed has dependenciesState_ being STALE, while all its observers have that being UP_TO_DATE. If I make its observer observe something else and trigger that thing, this computed gets back to a normal state and starts updating again. After further inspection, I believe this is caused by an action within a computed deep in the dependency chain. That action marks the computed POSSIBLY_STALE, but later the evaluation process marks all the observers UP_TO_DATE. The computed being POSSIBLY_STALE means no changes upstream would be propagated beyond the computed, and thus no observers would be triggered by the computed, and the computed gets stalled. This is a simplified reproduction of the issue: const counter = mobx.observable.box(0);const atom = mobx.createAtom('');const computed = mobx.computed(() => { atom.reportObserved(); mobx.runInAction(() => atom.reportChanged()); return counter.get();}, { equals: (a, b) => (Math.floor(a / 2) === Math.floor(b / 2)) }); export const App = observer(() => ( <button onClick={mobx.action(() => counter.set(counter.get() + 1))}> Inc {computed.get()} </button>)); The computed ought to output a result every second click to the button, but it never gets updated because it is trapped into the weird state mentioned above, and there is no error thrown for this. I fully understand that computed is not supposed to have side effect like this, and I totally agree with it and condemn that code. But in a sizable codebase and with a large development team, it is not always easy to capture such instances and enforce the rules without something more explicit. Having it fail silently also makes it very hard to diagnose. Should we consider maybe adding at least some DEV checks to detect such issue? — Reply to this email directly, view it on GitHub <#4544>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDVORRGREPCIW5RXDT2YWM5VAVCNFSM6AAAAAB22DKWVSVHI2DSMVQWIX3LMV43ERDJONRXK43TNFXW4OZYGE4DKNBRGY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[upsuper]

No I don't think there is any warning or so.

As I mentioned in the post:

I fully understand that computed is not supposed to have side effect like this, and I totally agree with it and condemn that code. But in a sizable codebase and with a large development team, it is not always easy to capture such instances and enforce the rules without something more explicit. Having it fail silently also makes it very hard to diagnose.

[mweststrate]

Sorry, I only saw half your message in my email reader originally, will check out why it doesn't warn as it does so in other similar cases

…

On Thu, Apr 10, 2025, 09:00 Xidorn Quan ***@***.***> wrote: No I don't think there is any warning or so. As I mentioned in the post: I fully understand that computed is not supposed to have side effect like this, and I totally agree with it and condemn that code. But in a sizable codebase and with a large development team, it is not always easy to capture such instances and enforce the rules without something more explicit. Having it fail silently also makes it very hard to diagnose. — Reply to this email directly, view it on GitHub <#4544 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBA33KZBOVGDM6KTKU32YYJKNAVCNFSM6AAAAAB22DKWVSVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTENZYG43DCMI> . You are receiving this because you commented.Message ID: ***@***.***>