fix: reschedule effect + avoid stale values#18189
Closed
dummdidumm wants to merge 3 commits into
Closed
Conversation
This fixes the failing redirect test in SvelteKit. It consists of two parts, both needed:
- reschedule new effects: when a batch commits, it needs to tell other batches about its new effects. We had this logic already but didn't apply it often enough. To avoid overfiring we add an additional set to track which of the new effects of a batch another batch already saw
- avoid persisting stale async values: when an async derived commits a value of a later batch, and then a value of an earlier batch is committed, we're persistent a stale value. We need to avoid that to not "go back in time"
- these two make the new test in this PR pass but not the redirect test in SvelteKit. For that I had to add the hack in `deriveds.js` to also unblock on the surrounding `flatten` effect
For the first part I also had a different approach, but it was more LOC and felt a bit scary with respects to deadlocks so I discarded it. In `deriveds.js` I had this before `decrement_pending?.()`:
```ts
if (!error) {
let blocked = false;
// All prior async derived runs are now stale
for (const [b, _d] of deferreds) {
if (b.id < batch.id) {
batch.unblocked.add(effect);
if (parent !== null) {
// Terrible hack: this way we unblock ourselves from `flatten`
// which can block us on initial run
batch.unblocked.add(parent);
}
batch.oncommit(() => _d.resolve(value));
if (batch.is_blocked_by(b)) {
// We do not want to commit just yet because it means we could write a newer
// value to the source before an older value, and then the older value is the
// latest one, creating stale UI / UI that "travels backwards".
const resume = () => queue_micro_task(() => handler(value, error));
_d.promise.then(resume, resume);
b.ondiscard(resume);
blocked = true;
}
}
}
if (blocked) return;
}
```
(and the related logic further down got removed)
🦋 Changeset detectedLatest commit: bb85f61 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 |
Contributor
|
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
6 tasks
Member
Author
|
Closing in favor of #18205 |
Rich-Harris
added a commit
that referenced
this pull request
May 13, 2026
This is another attempt to address some of the tricky edge cases that
arise when async batches resolve out of order. The idea is this:
Essentially, when a batch resolves:
1. we find the latest batch it shares changes with
2. if none exists (either because this is the earliest batch, or because
it is independent of any earlier batches):
- we commit it: effects are flushed, `oncommit` callbacks are run
- we restart any async work in later batches that depends on both the
committed values and the later batch's changes
3. otherwise, we don't commit the batch. instead:
- we merge the changes from the later batch onto the earlier batch. in
some cases this may mean restarting async work on the earlier batch, but
we avoid doing so unnecessarily.
- we then `#process()` the earlier batch. if it resolves, goto 1
This feels like it ought to work. There are still two failing tests,
which I'm currently looking into.
Notable changes:
- Instead of having a `batches` set, we have a linked list. When a batch
resolves, this makes it easy to find the batch that it should be merged
into
- The `#is_deferred` logic now takes account of skipped effects —
there's no need to wait for a promise inside a falsy `if` block (this
accounts for the change in the `async-inner-after-outer` test)
- We no longer care about `#blockers`
I would like to believe that this approach will allow us to simplify and
delete some code, for example the `rebase` logic, though that remains to
be seen. It also feels like some version of #18035 would be helpful.
- closes #18189
- closes #18162
- fixes sveltejs/kit#15431
### Before submitting the PR, please make sure you do the following
- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [x] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).
### Tests and linting
- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`
---------
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
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.
This fixes the failing redirect test in SvelteKit. It consists of two parts, both needed:
deriveds.jsto also unblock on the surroundingflatteneffectFor the first part I also had a different approach, but it was more LOC and felt a bit scary with respects to deadlocks so I discarded it. In
deriveds.jsI had this beforedecrement_pending?.():(and the related logic further down got removed)