fix: prevent flushed effects from running again#17787
Conversation
We never cleared the list of (maybe)dirty_effects on the assumption that once a batch has run them it's complete. But that's not the case when a boundary has a pending snippet, in which case the pending snippet shows up, so `blocking_pending` is already 0 and effects are flushed. That can lead to effects being run unnecessarily, even leading to infinite loops. So we clear them. This is safe because any additional effects would either be scheduled by the boundary (which keeps track of the offscreen effects created while the pending snippet is shown, and schedules them once the pending snippet goes away) or by unskipping skipped branches (which reschedules the effects inside it) Fixes #17717 After creating the test I noticed it fails when run together with other tests, but not alone, which lead me to discover that we're missing an `unset_context`. I also added clearing of `#skipped_branches` just to be safe.
🦋 Changeset detectedLatest commit: 61fcb6b 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 |
|
|
Thank you for working on this issue! I gave this PR a spin to see if this finally fixes the infinite loops, but almost immediately ran into a different issue: Navigating with (for context, the browser shows the correct new url, not the Feels more like a regression of this PR than a follow up issue as far as I can tell |
|
Please provide a reproduction. Are you maybe running into sveltejs/kit#15395 ? |
|
I'm working on a reproduction, but I have yet to actually achieve the same behaviour... so maybe it is something in user-land after all. If I do manage to create a reproduction, I'll open a new issue 👍 Its unlikely to be related to sveltejs/kit#15395, as I don't have SSR enabled in my app, but it I'll double check if its related to sveltejs/kit#14996 |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## svelte@5.53.4 ### Patch Changes - fix: set server context after async transformError ([#17799](#17799)) - fix: hydrate if blocks correctly ([#17784](#17784)) - fix: handle default parameters scope leaks ([#17788](#17788)) - fix: prevent flushed effects from running again ([#17787](#17787)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
@dummdidumm I actually managed to reproduce the bug: sveltejs/kit#15431 |
3 participants
We never cleared the list of (maybe)dirty_effects on the assumption that once a batch has run them it's complete. But that's not the case when a boundary has a pending snippet, in which case the pending snippet shows up, so
blocking_pendingis already 0 and effects are flushed. That can lead to effects being run unnecessarily, even leading to infinite loops.So we clear them. This is safe because any additional effects would either be scheduled by the boundary (which keeps track of the offscreen effects created while the pending snippet is shown, and schedules them once the pending snippet goes away) or by unskipping skipped branches (which reschedules the effects inside it)
Fixes #17717
After creating the test I noticed it fails when run together with other tests, but not alone, which lead me to discover that we're missing an
unset_context. I also added clearing of#skipped_branchesjust to be safe.