From 0adc22c9ae5f98718bf5b7b83753767cc919b0e1 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 18 Mar 2026 12:43:17 -0400 Subject: [PATCH] fix: defer batch resolution until earlier intersecting batches have committed (#17162) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes an awkward bug with `each` blocks containing `await`, especially keyed `each` blocks. If you do `array.push(...)` multiple times in distinct batches, something weird happens — to the `each` block, the array looks this... ```js [1] ``` ...then this... ```js [undefined, 2] ``` ...then this... ```js [undefined, undefined, 3] ``` ...and so on. That's because as far as Svelte's reactivity is concerned, what we're _really_ doing is assigning to `array[0]` then `array[1]` then `array[2]`. Those (along with `array.length`) are each backed by independent sources, which we can rewind individually when we need to 'apply' a batch. When it comes to sources that actually _are_ independent, this is useful, since we apply the changes from batch B to the DOM while we're still waiting for a promise in batch A to resolve. But in this case it's not ideal, because you would expect these changes to accumulate. In particular, this fails with keyed `each` blocks because duplicate keys are disallowed (assuming the key function doesn't break on `undefined` _before_ the duplicate check happens). This PR fixes it by stacking batches that are interconnected. Specifically, if a later batch has some (but not all) sources in common with an earlier batch, then when we apply the batch we include the sources from the earlier batch, and block it until the earlier batch commits. When the earlier batch commits, it will check to see if doing so unblocks any later batches, and if so process them. In the course of working on this I realised that `SvelteSet` and `SvelteMap` aren't async-ready — will follow up this PR with ones for those. Fixes #17050 --------- Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> Co-authored-by: Simon Holthausen --- .changeset/spicy-teeth-tan.md | 5 + .../src/internal/client/reactivity/async.js | 7 +- .../src/internal/client/reactivity/batch.js | 148 ++++++++++--- .../internal/client/reactivity/deriveds.js | 2 +- .../samples/async-derived-indirect/_config.js | 29 +++ .../async-derived-indirect/main.svelte | 21 ++ .../samples/async-each-overlap/_config.js | 102 --------- .../async-ignore-skipped-block/_config.js | 29 +++ .../async-ignore-skipped-block/main.svelte | 14 ++ .../async-later-sync-overlaps/_config.js | 30 +++ .../async-later-sync-overlaps/main.svelte | 17 ++ .../async-overlapping-array/_config.js | 205 ++++++++++++++++++ .../main.svelte | 8 +- 13 files changed, 474 insertions(+), 143 deletions(-) create mode 100644 .changeset/spicy-teeth-tan.md create mode 100644 packages/svelte/tests/runtime-runes/samples/async-derived-indirect/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-derived-indirect/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/async-each-overlap/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-overlapping-array/_config.js rename packages/svelte/tests/runtime-runes/samples/{async-each-overlap => async-overlapping-array}/main.svelte (84%) diff --git a/.changeset/spicy-teeth-tan.md b/.changeset/spicy-teeth-tan.md new file mode 100644 index 0000000000..c7efa65130 --- /dev/null +++ b/.changeset/spicy-teeth-tan.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: defer batch resolution until earlier intersecting batches have committed diff --git a/packages/svelte/src/internal/client/reactivity/async.js b/packages/svelte/src/internal/client/reactivity/async.js index 9b24400840..d713385c75 100644 --- a/packages/svelte/src/internal/client/reactivity/async.js +++ b/packages/svelte/src/internal/client/reactivity/async.js @@ -307,15 +307,16 @@ export function wait(blockers) { * @returns {(skip?: boolean) => void} */ export function increment_pending() { - var boundary = /** @type {Boundary} */ (/** @type {Effect} */ (active_effect).b); + var effect = /** @type {Effect} */ (active_effect); + var boundary = /** @type {Boundary} */ (effect.b); var batch = /** @type {Batch} */ (current_batch); var blocking = boundary.is_rendered(); boundary.update_pending_count(1, batch); - batch.increment(blocking); + batch.increment(blocking, effect); return (skip = false) => { boundary.update_pending_count(-1, batch); - batch.decrement(blocking, skip); + batch.decrement(blocking, effect, skip); }; } diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index b100d559d2..3b10d6ebe6 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -90,20 +90,20 @@ var source_stacks = DEV ? new Set() : null; let uid = 1; export class Batch { - // for debugging. TODO remove once async is stable id = uid++; /** - * The current values of any sources that are updated in this batch + * The current values of any signals that are updated in this batch. + * Tuple format: [value, is_derived] (note: is_derived is false for deriveds, too, if they were overridden via assignment) * They keys of this map are identical to `this.#previous` - * @type {Map} + * @type {Map} */ current = new Map(); /** - * The values of any sources that are updated in this batch _before_ those updates took place. + * The values of any signals (sources and deriveds) that are updated in this batch _before_ those updates took place. * They keys of this map are identical to `this.#current` - * @type {Map} + * @type {Map} */ previous = new Map(); @@ -121,14 +121,16 @@ export class Batch { #discard_callbacks = new Set(); /** - * The number of async effects that are currently in flight + * Async effects that are currently in flight + * @type {Map} */ - #pending = 0; + #pending = new Map(); /** - * The number of async effects that are currently in flight, _not_ inside a pending boundary + * Async effects that are currently in flight, _not_ inside a pending boundary + * @type {Map} */ - #blocking_pending = 0; + #blocking_pending = new Map(); /** * A deferred that resolves when the batch is committed, used with `settled()` @@ -168,8 +170,35 @@ export class Batch { #decrement_queued = false; + /** @type {Set} */ + #blockers = new Set(); + #is_deferred() { - return this.is_fork || this.#blocking_pending > 0; + return this.is_fork || this.#blocking_pending.size > 0; + } + + #is_blocked() { + for (const batch of this.#blockers) { + for (const effect of batch.#blocking_pending.keys()) { + var skipped = false; + var e = effect; + + while (e.parent !== null) { + if (this.#skipped_branches.has(e)) { + skipped = true; + break; + } + + e = e.parent; + } + + if (!skipped) { + return true; + } + } + } + + return false; } /** @@ -264,7 +293,7 @@ export class Batch { collected_effects = null; legacy_updates = null; - if (this.#is_deferred()) { + if (this.#is_deferred() || this.#is_blocked()) { this.#defer_effects(render_effects); this.#defer_effects(effects); @@ -272,7 +301,7 @@ export class Batch { reset_branch(e, t); } } else { - if (this.#pending === 0) { + if (this.#pending.size === 0) { batches.delete(this); } @@ -383,17 +412,18 @@ export class Batch { /** * Associate a change to a given source with the current * batch, noting its previous and current values - * @param {Source} source + * @param {Value} source * @param {any} old_value + * @param {boolean} [is_derived] */ - capture(source, old_value) { + capture(source, old_value, is_derived = false) { if (old_value !== UNINITIALIZED && !this.previous.has(source)) { this.previous.set(source, old_value); } // Don't save errors in `batch_values`, or they won't be thrown in `runtime.js#get` if ((source.f & ERROR_VALUE) === 0) { - this.current.set(source, source.v); + this.current.set(source, [source.v, is_derived]); batch_values?.set(source, source.v); } } @@ -453,11 +483,13 @@ export class Batch { /** @type {Source[]} */ var sources = []; - for (const [source, value] of this.current) { + for (const [source, [value, is_derived]] of this.current) { if (batch.current.has(source)) { - if (is_earlier && value !== batch.current.get(source)) { + var batch_value = /** @type {[any, boolean]} */ (batch.current.get(source))[0]; // faster than destructuring + + if (is_earlier && value !== batch_value) { // bring the value up to date - batch.current.set(source, value); + batch.current.set(source, [value, is_derived]); } else { // same value or later batch has more recent value, // no need to re-run these effects @@ -507,24 +539,56 @@ export class Batch { batch.deactivate(); } } + + for (const batch of batches) { + if (batch.#blockers.has(this)) { + batch.#blockers.delete(this); + + if (batch.#blockers.size === 0 && !batch.#is_deferred()) { + batch.activate(); + batch.#process(); + } + } + } } /** - * * @param {boolean} blocking + * @param {Effect} effect */ - increment(blocking) { - this.#pending += 1; - if (blocking) this.#blocking_pending += 1; + increment(blocking, effect) { + let pending_count = this.#pending.get(effect) ?? 0; + this.#pending.set(effect, pending_count + 1); + + if (blocking) { + let blocking_pending_count = this.#blocking_pending.get(effect) ?? 0; + this.#blocking_pending.set(effect, blocking_pending_count + 1); + } } /** * @param {boolean} blocking + * @param {Effect} effect * @param {boolean} skip - whether to skip updates (because this is triggered by a stale reaction) */ - decrement(blocking, skip) { - this.#pending -= 1; - if (blocking) this.#blocking_pending -= 1; + decrement(blocking, effect, skip) { + let pending_count = this.#pending.get(effect) ?? 0; + + if (pending_count === 1) { + this.#pending.delete(effect); + } else { + this.#pending.set(effect, pending_count - 1); + } + + if (blocking) { + let blocking_pending_count = this.#blocking_pending.get(effect) ?? 0; + + if (blocking_pending_count === 1) { + this.#blocking_pending.delete(effect); + } else { + this.#blocking_pending.set(effect, blocking_pending_count - 1); + } + } if (this.#decrement_queued || skip) return; this.#decrement_queued = true; @@ -597,15 +661,37 @@ export class Batch { // if there are multiple batches, we are 'time travelling' — // we need to override values with the ones in this batch... - batch_values = new Map(this.current); + batch_values = new Map(); + for (const [source, [value]] of this.current) { + batch_values.set(source, value); + } - // ...and undo changes belonging to other batches + // ...and undo changes belonging to other batches unless they block this one for (const batch of batches) { if (batch === this || batch.is_fork) continue; - for (const [source, previous] of batch.previous) { - if (!batch_values.has(source)) { - batch_values.set(source, previous); + // A batch is blocked on an earlier batch if it overlaps with the earlier batch's changes but is not a superset + var intersects = false; + var differs = false; + + if (batch.id < this.id) { + for (const [source, [, is_derived]] of batch.current) { + // Derived values don't partake in the blocking mechanism, because a derived could + // be triggered in one batch already but not the other one yet, causing a false-positive + if (is_derived) continue; + + intersects ||= this.current.has(source); + differs ||= !this.current.has(source); + } + } + + if (intersects && differs) { + this.#blockers.add(batch); + } else { + for (const [source, previous] of batch.previous) { + if (!batch_values.has(source)) { + batch_values.set(source, previous); + } } } } @@ -1065,7 +1151,7 @@ export function fork(fn) { batch.is_fork = false; // apply changes and update write versions so deriveds see the change - for (var [source, value] of batch.current) { + for (var [source, [value]] of batch.current) { source.v = value; source.wv = increment_write_version(); } diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 7b932a8356..5da0df0670 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -396,7 +396,7 @@ export function update_derived(derived) { // change, `derived.equals` may incorrectly return `true` if (!current_batch?.is_fork || derived.deps === null) { derived.v = value; - current_batch?.capture(derived, old_value); + current_batch?.capture(derived, old_value, true); // deriveds without dependencies should never be recomputed if (derived.deps === null) { diff --git a/packages/svelte/tests/runtime-runes/samples/async-derived-indirect/_config.js b/packages/svelte/tests/runtime-runes/samples/async-derived-indirect/_config.js new file mode 100644 index 0000000000..9d97b736be --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-derived-indirect/_config.js @@ -0,0 +1,29 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await tick(); + const [a, b, resolve] = target.querySelectorAll('button'); + + a.click(); + await tick(); + b.click(); + await tick(); + resolve.click(); + await tick(); + resolve.click(); + await tick(); + + assert.htmlEqual( + target.innerHTML, + ` + + + + hi + 1 + ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-derived-indirect/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-derived-indirect/main.svelte new file mode 100644 index 0000000000..f0d3bdc809 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-derived-indirect/main.svelte @@ -0,0 +1,21 @@ + + + + + + +{#if a_b}hi{/if} +{await push(a_b)} diff --git a/packages/svelte/tests/runtime-runes/samples/async-each-overlap/_config.js b/packages/svelte/tests/runtime-runes/samples/async-each-overlap/_config.js deleted file mode 100644 index d03f9ad09d..0000000000 --- a/packages/svelte/tests/runtime-runes/samples/async-each-overlap/_config.js +++ /dev/null @@ -1,102 +0,0 @@ -import { tick } from 'svelte'; -import { test } from '../../test'; - -export default test({ - skip: true, - async test({ assert, target }) { - const [add, shift] = target.querySelectorAll('button'); - - add.click(); - await tick(); - add.click(); - await tick(); - add.click(); - await tick(); - - // TODO pending count / number of pushes is off - - assert.htmlEqual( - target.innerHTML, - ` - - -

pending=6 values.length=1 values=[1]

-
not keyed: -
1
-
-
keyed: -
1
-
- ` - ); - - shift.click(); - await tick(); - shift.click(); - await tick(); - assert.htmlEqual( - target.innerHTML, - ` - - -

pending=4 values.length=2 values=[1,2]

-
not keyed: -
1
-
2
-
-
keyed: -
1
-
2
-
- ` - ); - - shift.click(); - await tick(); - shift.click(); - await tick(); - assert.htmlEqual( - target.innerHTML, - ` - - -

pending=2 values.length=3 values=[1,2,3]

-
not keyed: -
1
-
2
-
3
-
-
keyed: -
1
-
2
-
3
-
- ` - ); - - shift.click(); - await tick(); - shift.click(); - await tick(); - assert.htmlEqual( - target.innerHTML, - ` - - -

pending=0 values.length=4 values=[1,2,3,4]

-
not keyed: -
1
-
2
-
3
-
4
-
-
keyed: -
1
-
2
-
3
-
4
-
- ` - ); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/_config.js b/packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/_config.js new file mode 100644 index 0000000000..1d1a489d67 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/_config.js @@ -0,0 +1,29 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await tick(); + + const [a, b] = target.querySelectorAll('button'); + + assert.htmlEqual(target.innerHTML, `

hello

`); + + a.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `

hello

`); + + a.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `

hello

`); + + a.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `

hello

`); + + // if we don't skip over the never-resolving promise in the `else` block, we will never update + b.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `

hello

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/main.svelte new file mode 100644 index 0000000000..4ef0694d40 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-ignore-skipped-block/main.svelte @@ -0,0 +1,14 @@ + + + + + +{#if show} +

hello

+{:else} + {await new Promise(() => {})} +{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/_config.js b/packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/_config.js new file mode 100644 index 0000000000..a65c8a6fba --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/_config.js @@ -0,0 +1,30 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await tick(); + const [a_b, b, resolve] = target.querySelectorAll('button'); + + a_b.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ' 0' + ); + + b.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ' 0' + ); + + resolve.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ' 1' + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/main.svelte new file mode 100644 index 0000000000..8a9e2a866c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-later-sync-overlaps/main.svelte @@ -0,0 +1,17 @@ + + + + + +{await push(a)} diff --git a/packages/svelte/tests/runtime-runes/samples/async-overlapping-array/_config.js b/packages/svelte/tests/runtime-runes/samples/async-overlapping-array/_config.js new file mode 100644 index 0000000000..2dad5babea --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-overlapping-array/_config.js @@ -0,0 +1,205 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await tick(); + + const [add, shift, pop] = target.querySelectorAll('button'); + + add.click(); + await tick(); + add.click(); + await tick(); + add.click(); + await tick(); + + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=6 values.length=1 values=[1]

+
not keyed: +
1
+
+
keyed: +
1
+
+ ` + ); + + shift.click(); + await tick(); + shift.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=4 values.length=2 values=[1,2]

+
not keyed: +
1
+
2
+
+
keyed: +
1
+
2
+
+ ` + ); + + shift.click(); + await tick(); + shift.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=2 values.length=3 values=[1,2,3]

+
not keyed: +
1
+
2
+
3
+
+
keyed: +
1
+
2
+
3
+
+ ` + ); + + shift.click(); + await tick(); + shift.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=0 values.length=4 values=[1,2,3,4]

+
not keyed: +
1
+
2
+
3
+
4
+
+
keyed: +
1
+
2
+
3
+
4
+
+ ` + ); + + add.click(); + await tick(); + add.click(); + await tick(); + add.click(); + await tick(); + add.click(); + await tick(); + + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=8 values.length=4 values=[1,2,3,4]

+
not keyed: +
1
+
2
+
3
+
4
+
+
keyed: +
1
+
2
+
3
+
4
+
+ ` + ); + + // pop should have no effect until earlier promises have also resolved + pop.click(); + await tick(); + pop.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=6 values.length=4 values=[1,2,3,4]

+
not keyed: +
1
+
2
+
3
+
4
+
+
keyed: +
1
+
2
+
3
+
4
+
+ ` + ); + + pop.click(); + await tick(); + pop.click(); + await tick(); + pop.click(); + await tick(); + pop.click(); + await tick(); + pop.click(); + await tick(); + pop.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + + +

pending=0 values.length=8 values=[1,2,3,4,5,6,7,8]

+
not keyed: +
1
+
2
+
3
+
4
+
5
+
6
+
7
+
8
+
+
keyed: +
1
+
2
+
3
+
4
+
5
+
6
+
7
+
8
+
+ ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-each-overlap/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-overlapping-array/main.svelte similarity index 84% rename from packages/svelte/tests/runtime-runes/samples/async-each-overlap/main.svelte rename to packages/svelte/tests/runtime-runes/samples/async-overlapping-array/main.svelte index af9d395457..e41b6926b8 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-each-overlap/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/async-overlapping-array/main.svelte @@ -11,18 +11,14 @@ return p.promise; } - function shift() { - const fn = queue.shift(); - if (fn) fn(); - } - function addValue() { values.push(values.length+1); } - + +

pending={$effect.pending()}