diff --git a/.changeset/fix-endbatch-unobservation-reentrancy.md b/.changeset/fix-endbatch-unobservation-reentrancy.md new file mode 100644 index 000000000..9ebd53c29 --- /dev/null +++ b/.changeset/fix-endbatch-unobservation-reentrancy.md @@ -0,0 +1,5 @@ +--- +"mobx": patch +--- + +Fix a stack overflow ("Maximum call stack size exceeded") that could occur when an `onBecomeUnobserved` handler disposes a `Reaction`. Disposing a `Reaction` re-enters `endBatch()`, which used to recurse into the same `pendingUnobservations` drain loop instead of letting the already-running outer loop pick up the newly queued items, causing unbounded stack depth for long enough chains. diff --git a/packages/mobx/__tests__/base/become-observed.ts b/packages/mobx/__tests__/base/become-observed.ts index 8222cdbd0..b7cdabb4f 100644 --- a/packages/mobx/__tests__/base/become-observed.ts +++ b/packages/mobx/__tests__/base/become-observed.ts @@ -510,6 +510,58 @@ test("#2667", () => { ]) }) +test("#3954 - disposing a chain of reactions from onBecomeUnobserved doesn't overflow the stack", () => { + // Each box's onBecomeUnobserved handler disposes the next reaction in the + // chain. Disposing reaction[0] unobserves box[0], whose handler disposes + // reaction[1], which unobserves box[1], and so on. Each of those disposals + // re-enters endBatch() while the previous one is still draining + // pendingUnobservations, so this used to recurse N deep instead of + // looping, overflowing the stack for a large enough chain. + const N = 10000 + const boxes = Array.from({ length: N }, () => observable.box(0)) + const disposers = boxes.map(box => autorun(() => box.get())) + let unobservedCount = 0 + + boxes.forEach((box, i) => { + onBecomeUnobserved(box, () => { + unobservedCount++ + if (i + 1 < N) { + disposers[i + 1]() + } + }) + }) + + expect(() => disposers[0]()).not.toThrow() + + // the whole chain should have unwound, not just the first link + expect(unobservedCount).toBe(N) +}) + +test("#3954 followup - isRunningUnobservations is released even if an onBecomeUnobserved handler throws", () => { + const boxA = observable.box(0) + const disposeA = autorun(() => boxA.get()) + onBecomeUnobserved(boxA, () => { + throw new Error("boom") + }) + + // the handler's exception should still surface to the caller, not be swallowed + expect(() => disposeA()).toThrow("boom") + + // if the internal guard were left stuck true after that exception, every + // future endBatch() would silently stop draining pendingUnobservations, + // so this completely unrelated disposal would never fire its own handler + const boxB = observable.box(0) + const disposeB = autorun(() => boxB.get()) + let unobservedB = false + onBecomeUnobserved(boxB, () => { + unobservedB = true + }) + + disposeB() + + expect(unobservedB).toBe(true) +}) + test("works with ObservableSet #3595", () => { const onSetObserved = jest.fn() const onSetUnobserved = jest.fn() diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index 35de9dfea..9abf194e0 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/packages/mobx/src/core/globalstate.ts @@ -81,6 +81,14 @@ export class MobXGlobals { */ isRunningReactions = false + /** + * Are we currently draining pendingUnobservations in endBatch? + * An onBecomeUnobserved handler can dispose a Reaction, which calls + * startBatch/endBatch again; this guards against re-entering the same + * drain loop recursively (see endBatch in observable.ts). + */ + isRunningUnobservations = false + /** * Is it allowed to change observables at this point? * In general, MobX doesn't allow that when running computations and React.render. diff --git a/packages/mobx/src/core/observable.ts b/packages/mobx/src/core/observable.ts index 5ddbc74c8..7d2361309 100644 --- a/packages/mobx/src/core/observable.ts +++ b/packages/mobx/src/core/observable.ts @@ -111,24 +111,39 @@ export function endBatch() { if (--globalState.inBatch === 0) { runReactions() // the batch is actually about to finish, all unobserving should happen here. - const list = globalState.pendingUnobservations - for (let i = 0; i < list.length; i++) { - const observable = list[i] - observable.isPendingUnobservation = false - if (observable.observers_.size === 0) { - if (observable.isBeingObserved) { - // if this observable had reactive observers, trigger the hooks - observable.isBeingObserved = false - observable.onBUO() - } - if (observable instanceof ComputedValue) { - // computed values are automatically teared down when the last observer leaves - // this process happens recursively, this computed might be the last observabe of another, etc.. - observable.suspend_() + // Guard against re-entering this loop: an onBUO handler can dispose a Reaction, + // which calls startBatch/endBatch again while we're still iterating. Bail out of + // the nested call instead of recursing; the outer loop re-reads list.length on + // every iteration, so it picks up anything the nested dispose() pushes onto the + // same pendingUnobservations array. + if (!globalState.isRunningUnobservations) { + globalState.isRunningUnobservations = true + try { + const list = globalState.pendingUnobservations + for (let i = 0; i < list.length; i++) { + const observable = list[i] + observable.isPendingUnobservation = false + if (observable.observers_.size === 0) { + if (observable.isBeingObserved) { + // if this observable had reactive observers, trigger the hooks + observable.isBeingObserved = false + observable.onBUO() + } + if (observable instanceof ComputedValue) { + // computed values are automatically teared down when the last observer leaves + // this process happens recursively, this computed might be the last observabe of another, etc.. + observable.suspend_() + } + } } + globalState.pendingUnobservations = [] + } finally { + // Always release the guard, even if an onBUO handler (user code) threw, + // otherwise every future endBatch() would see isRunningUnobservations + // stuck true and silently stop draining pendingUnobservations forever. + globalState.isRunningUnobservations = false } } - globalState.pendingUnobservations = [] } }