fix(core): stop running further effects once one destroys the view mid-flush - #65698
fix(core): stop running further effects once one destroys the view mid-flush#65698arturovt wants to merge 1 commit into
Conversation
|
If this is the same issue as #62822, this is probably not the fix we want. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Chatted offline with Arturo. |
2d7d534 to
1d6234e
Compare
|
(can you please resolve the conflict ?) |
1d6234e to
1f153cd
Compare
|
TGP looks "green" to me |
1f153cd to
929c312
Compare
|
I think @alxhub should probably review this |
929c312 to
13ff889
Compare
13ff889 to
59d548e
Compare
59d548e to
6ca9e0e
Compare
atscott
left a comment
There was a problem hiding this comment.
AGENT: Thanks for working on this.
Please scope this PR strictly to the issue reported in #62822 (TypeError: view[EFFECTS] is not iterable).
-
runEffectsInViewloop guard: Checkingview[EFFECTS] === nullonly at thewhileloop level is insufficient. If a view has multiple dirty effects and one destroys the view,view[EFFECTS]is set tonullmid-iteration, but thefor...ofloop continues and executes subsequent effects on the destroyed view. The bail-out check must be inside theforloop so execution stops immediately. -
Revert the cleanup changes in
effect.ts: Destroying a component/view from within its own effect and then continuing execution to register cleanups is an antipattern. If an effect triggers the destruction of its host, it must not continue executing or setting up teardown logic for work on a dead component. ModifyingonCleanupto run callbacks synchronously inline upon registration to accommodate this fundamentally breaks the contract and mental model ofonCleanup. We will not add framework complexity or workarounds to support this pattern.
6ca9e0e to
ee84e72
Compare
atscott
left a comment
There was a problem hiding this comment.
AGENT: Thanks for updating this!
Two remaining items to address:
-
runEffectsInViewstill crashes if thewhileloop restarts:
Moving the check inside thefor...ofloop protects against subsequent effects in the current iteration, but iftryFlushEffectsevaluates totrue(e.g. an effect updated a signal that dirtied a child view effect before destroying the view), thewhileloop re-enters. At that point, JavaScript tries to evaluatefor (const effect of view[EFFECTS])onnulland throwsTypeError: view[EFFECTS] is not iterablebefore entering the loop.We can fix this by returning immediately after
effect.run()ifview[EFFECTS] === null(or checking in thewhilecondition). Please also add a test covering this scenario (an effect dirtying another effect while destroying the view). -
Remove the documentation / JSDoc additions:
We generally avoid documenting obscure edge cases in the general guides and public JSDocs as it adds unnecessary complexity to the documentation. Let's remove the changes inadev/src/content/guide/signals/effect.mdandpackages/core/src/render3/reactivity/effect.ts.
ee84e72 to
888d48e
Compare
…d-flush When a view has more than one effect scheduled to run, and one of them destroys the view (e.g. by calling `componentRef.destroy()`), the remaining effects in that same flush could still run afterward, against a view that no longer exists. In some cases this crashed outright with `TypeError: view[EFFECTS] is not iterable`. Here's why: `runEffectsInView` walks a view's effects in a `for...of` loop, wrapped in an outer `while` loop that re-checks for any effects that became dirty as a side effect of ones that already ran. When an effect destroys its view, `view[EFFECTS]` gets set to `null` as part of tearing the view down. First attempt checked for that inside the `for...of` loop, before each effect runs. That covers a sibling effect later in the *same* pass, but misses a second case: if the effect that destroys the view *also* dirties another effect on that same view in the process (e.g. by writing a signal the sibling depends on), the outer `while` loop sees `HasChildViewsToRefresh` set and tries to restart — and immediately crashes re-entering `for (const effect of view[EFFECTS])` on a now-null value, before the in-loop check ever gets a chance to run. Reproduced that exact crash with a test first: two effects on one view, the second one writes a signal the first depends on and then destroys the view in the same call — confirmed it throws before touching the fix. Fixed by checking right after `effect.run()` instead of before it, covering both cases in one place: the remaining effects in the current pass, and the loop trying to restart afterward. As soon as one effect destroys the view, nothing else runs against it again. This is intentionally narrow in scope. An earlier version of this fix also tried to guarantee that `onCleanup()` callbacks still ran even when registered after an effect destroyed its own view. That's been dropped — destroying your own view and then continuing to register more work for it isn't something the framework should have to paper over. If you need to do both, register `onCleanup` first, then destroy.
888d48e to
974d588
Compare
When a view has more than one effect scheduled to run, and one of them
destroys the view (e.g. by calling
componentRef.destroy()), theremaining effects in that same flush could still run afterward,
against a view that no longer exists. In some cases this crashed
outright with
TypeError: view[EFFECTS] is not iterable.Here's why:
runEffectsInViewwalks a view's effects in afor...ofloop, wrapped in an outer
whileloop that re-checks for any effectsthat became dirty as a side effect of ones that already ran. When an
effect destroys its view,
view[EFFECTS]gets set tonullas partof tearing the view down.
First attempt checked for that inside the
for...ofloop, before eacheffect runs. That covers a sibling effect later in the same pass,
but misses a second case: if the effect that destroys the view also
dirties another effect on that same view in the process (e.g. by
writing a signal the sibling depends on), the outer
whileloop seesHasChildViewsToRefreshset and tries to restart — and immediatelycrashes re-entering
for (const effect of view[EFFECTS])on anow-null value, before the in-loop check ever gets a chance to run.
Reproduced that exact crash with a test first: two effects on one
view, the second one writes a signal the first depends on and then
destroys the view in the same call — confirmed it throws before
touching the fix.
Fixed by checking right after
effect.run()instead of before it,covering both cases in one place: the remaining effects in the current
pass, and the loop trying to restart afterward. As soon as one effect
destroys the view, nothing else runs against it again.
This is intentionally narrow in scope. An earlier version of this fix
also tried to guarantee that
onCleanup()callbacks still ran evenwhen registered after an effect destroyed its own view. That's been
dropped — destroying your own view and then continuing to register
more work for it isn't something the framework should have to paper
over. If you need to do both, register
onCleanupfirst, thendestroy.