Fix nested-effect cleanup order and add LIFO disposal#116
Merged
Conversation
…anup Two bugs in cleanup of nested effects: 1. When an inner effect is auto-disposed (parent re-run / parent dispose / scope dispose), its cleanup was never called. The unwatched callback routed straight to effectScopeOper, which clears state but never reads .cleanup. Inner cleanup was silently dropped on every auto-dispose path. 2. Even after wiring cleanup into the auto-dispose path, the order was inverted: the parent's own cleanup ran first (in effectOper / run() / updateComputed), then children were disposed via purgeDeps at the end. Standard reverse-of-creation order needs the inner cleanup to run BEFORE the outer cleanup, so the inner can still rely on whatever state the outer set up; and BEFORE the body re-runs (in the re-run case), so the old inner doesn't end up cleaning up state that the new inner already set up. Fixes: - unwatched() dispatches by node shape: EffectNode → effectOper (full dispose including cleanup), ComputedNode → its existing stale-clear path, EffectScopeNode → effectScopeOper. - effectOper / effectScopeOper / run() / updateComputed all walk this.depsTail backward and unlink child EffectNode deps before the node's own cleanup runs. unlink triggers unwatched → recursive effectOper on each child, so grandchildren get cleaned up depth-first reverse before their parent. - A per-effect flag (HasChildEffect, defined locally in index.ts — outside ReactiveFlags' range so system.ts never touches it) gates the pre-walk in run() and updateComputed. Leaf effects with no child effects skip the walk entirely; the flag is set by effect() when it links to a parent and is naturally cleared at the start of a body re-run. - The not-dirty branch of run() (restore Watching after notify chain) explicitly preserves HasChildEffect: e.flags = Watching | (flags & HasChildEffect). Otherwise an inner-only re-run would clobber the outer's bit and the next real re-run would skip the pre-walk. Tests: - effect.spec.ts: - cleanup order on outer re-run - cleanup order on dispose - sibling cleanup on dispose / re-run (LIFO) - three-level nested cleanup - cleanup order after a prior inner-only re-run - effect created inside computed (parent is ComputedNode) - effectScope.spec.ts: - scope dispose runs child cleanup - sibling LIFO on scope dispose - depth-first reverse for nested in scope All 198 pass.
|
|
Bug 002 — effectScope as intermediate parent:
effectScope() linked itself to its parent without setting HasChildEffect
on the parent, so when the chain was effect → effectScope → effect, the
outer's HasChildEffect bit was never set and run()'s pre-walk was
skipped. The old inner's cleanup then fired after the new inner had
already been set up. Mirror effect()'s flag-set inside effectScope().
Additionally, run()'s and updateComputed's pre-walks only unlinked deps
matching `'fn' in dep`, so even with the flag set the loop skipped scope
deps. Broaden the predicate to `!('getter' in dep) && !('currentValue'
in dep)` to also match EffectScopeNode — unlinking a scope cascades
through unwatched → effectScopeOper, which already disposes the scope's
own deps in reverse.
Bug 001 — computed unwatched FIFO:
When a computed lost its last subscriber, the 'getter' in node branch
of unwatched routed through purgeDeps, which walks deps from head to
tail (FIFO). If the computed's getter had created multiple child
effects, their cleanups ran in creation order instead of LIFO.
Replace purgeDeps with a depsTail-backward walk in that branch.
Added regression tests:
- tests/effect.spec.ts: computed unwatched LIFO ordering.
- tests/effectScope.spec.ts: scope-as-intermediate cleanup order.
All 200 pass.
The new reverse-walk inside the 'getter' in node branch confused tsslint's type inference because `node.depsTail` is narrowed to `Link` after the `!== undefined` check, but the loop variable needs to accept `Link | undefined` for the `link = prev` step. Import `Link` from system.js and annotate both `link` and `prev` explicitly.
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.
Summary
Two bugs in cleanup of nested effects:
Inner cleanup never ran on auto-dispose. When an inner effect was disposed by the parent re-running or being disposed (rather than by the user calling its own dispose function),
unwatchedrouted it straight toeffectScopeOper, which only clears state —.cleanupwas never read. Inner cleanup was silently dropped on every auto-dispose path.Order was inverted. Even with cleanup hooked into auto-dispose, the parent's own cleanup ran first; children were disposed afterwards via
purgeDeps. Standard reverse-of-creation order requires:Changes
Routing —
unwatcheddispatches by node shapePre-walk children before own cleanup
effectOper,effectScopeOper,run(), andupdateComputedall walkthis.depsTailbackward andunlinkchild EffectNode deps before the node's own cleanup runs.unlinktriggersunwatched→ recursiveeffectOperon each child, so grandchildren clean up depth-first reverse before their parent.Fast path —
HasChildEffectflagA new bit defined locally in
index.ts(outsideReactiveFlags' range sosystem.tsnever touches it).effect()sets it on the parent when linking;run()andupdateComputedcheck it before doing the pre-walk. Leaf effects with no child effects skip the walk entirely.The flag is dynamic — it's cleared at the start of every body re-run and re-set if the body creates new children. So an effect that stops creating children moves back to the fast path automatically.
The
else ifbranch ofrun()(the parent-chain-notify case where outer is queued but not dirty) explicitly preserves the flag:e.flags = Watching | (flags & HasChildEffect). Otherwise an inner-only re-run clobbers the outer's bit and the next real re-run skips the pre-walk.Tests
tests/effect.spec.ts:updateComputedregression)tests/effectScope.spec.ts(new):198 passing.