Fix stack overflow in endBatch() when disposing a Reaction from onBecomeUnobserved#4684
Open
a-y-ibrahim wants to merge 2 commits into
Open
Fix stack overflow in endBatch() when disposing a Reaction from onBecomeUnobserved#4684a-y-ibrahim wants to merge 2 commits into
a-y-ibrahim wants to merge 2 commits into
Conversation
…omeUnobserved endBatch()'s pendingUnobservations drain loop calls onBUO() per observable, which can dispose a Reaction (a common pattern). Reaction.dispose() calls startBatch()/endBatch() again, and since inBatch is already back at 0 at that point, the nested endBatch() recursed into the same drain loop instead of returning, unlike runReactions() which already guards against this exact kind of reentrancy with isRunningReactions. For long enough chains of onBecomeUnobserved handlers disposing reactions, this recursion overflowed the stack (issue mobxjs#3954). Add an analogous isRunningUnobservations guard: a nested endBatch() call now just returns, and the already-running outer loop picks up anything a nested dispose() pushes onto pendingUnobservations because it re-reads list.length on every iteration.
🦋 Changeset detectedLatest commit: af07124 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR fixes a stack overflow caused by re-entrant endBatch() calls when onBecomeUnobserved handlers dispose Reactions, by adding a guard to prevent recursive draining of pendingUnobservations.
Changes:
- Add
globalState.isRunningUnobservationsand use it inendBatch()to avoid recursive pending-unobservation draining. - Add a regression test reproducing the deep
onBecomeUnobserved -> dispose -> endBatchchain scenario from #3954. - Add a changeset entry for the patch release note.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/mobx/src/core/observable.ts | Adds a re-entrancy guard around pendingUnobservations draining inside endBatch(). |
| packages/mobx/src/core/globalstate.ts | Introduces isRunningUnobservations on MobXGlobals to support the new guard. |
| packages/mobx/tests/base/become-observed.ts | Adds a regression test for deep disposal chains triggered from onBecomeUnobserved. |
| .changeset/fix-endbatch-unobservation-reentrancy.md | Adds a patch-level changeset describing the stack overflow fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+119
to
140
| if (!globalState.isRunningUnobservations) { | ||
| globalState.isRunningUnobservations = true | ||
| 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 = [] | ||
| globalState.isRunningUnobservations = false | ||
| } |
Author
There was a problem hiding this comment.
Good catch, fixed in af07124: wrapped the drain in try/finally so isRunningUnobservations always releases, added a regression test (handler throws + a following unrelated disposal still fires normally) to lock this in.
If an onBecomeUnobserved handler threw, the guard flag never got reset, permanently disabling pendingUnobservations draining for the rest of the process. Wrap the drain in try/finally so it releases regardless of how the block exits; pendingUnobservations itself is only cleared on the success path, unchanged from before this fix, so a thrown exception still leaves the array for a later pass to pick back up. Added a regression test: a handler that throws still surfaces the error, and a second, unrelated disposal right after still fires its own handler instead of silently no-oping forever.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3954.
endBatch()'spendingUnobservationsdrain loop callsonBUO()per observable, which can dispose aReaction(a common pattern, e.g. tearing down a subscription once nothing observes it anymore).Reaction.dispose()callsstartBatch()/endBatch()again, and sinceinBatchis already back at 0 at that point, the nestedendBatch()recursed into the same drain loop instead of returning, unlikerunReactions()which already guards against this exact kind of reentrancy withisRunningReactions. For long enough chains ofonBecomeUnobservedhandlers disposing reactions, this recursion overflowed the stack.This adds an analogous
isRunningUnobservationsguard: a nestedendBatch()call now just returns, and the already-running outer loop picks up anything a nesteddispose()pushes ontopendingUnobservations, since it re-readslist.lengthon every iteration.Code change checklist
mainwithRangeError: Maximum call stack size exceeded, passes with this change)/docs. No public API change, so nothing to document.npm -w mobx run test:performance, both proxy and legacy modes, 37/37 passing with no timing anomalies)