fix: defer batch resolution until earlier intersecting batches have committed (#17162)

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 <simon.holthausen@vercel.com>
pull/17966/head
Rich Harris 5 months ago committed by GitHub
parent 7ec156a7b0
commit 0adc22c9ae
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: defer batch resolution until earlier intersecting batches have committed

@ -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);
};
}

@ -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<Source, any>}
* @type {Map<Value, [any, boolean]>}
*/
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<Source, any>}
* @type {Map<Value, any>}
*/
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<Effect, number>}
*/
#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<Effect, number>}
*/
#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<Batch>} */
#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();
}

@ -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) {

@ -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,
`
<button>a</button>
<button>b</button>
<button>resolve</button>
hi
1
`
);
}
});

@ -0,0 +1,21 @@
<script>
let a = $state(0);
let b = $state(0);
let a_b = $derived(a * b);
const queued = [];
function push(value) {
if (!value) return value;
return new Promise(resolve => {
queued.push(() => resolve(value));
});
}
</script>
<button onclick={() => (a++)}>a</button>
<button onclick={() => (b++)}>b</button>
<button onclick={() => (queued.shift()?.())}>resolve</button>
<!-- a_b called in a block effect before being called in an async effect -->
{#if a_b}hi{/if}
{await push(a_b)}

@ -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,
`
<button>add</button>
<button>shift</button>
<p>pending=6 values.length=1 values=[1]</p>
<div>not keyed:
<div>1</div>
</div>
<div>keyed:
<div>1</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=4 values.length=2 values=[1,2]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=2 values.length=3 values=[1,2,3]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=0 values.length=4 values=[1,2,3,4]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
`
);
}
});

@ -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, `<button>a 0</button><button>b 0</button><p>hello</p>`);
a.click();
await tick();
assert.htmlEqual(target.innerHTML, `<button>a 0</button><button>b 0</button><p>hello</p>`);
a.click();
await tick();
assert.htmlEqual(target.innerHTML, `<button>a 2</button><button>b 0</button><p>hello</p>`);
a.click();
await tick();
assert.htmlEqual(target.innerHTML, `<button>a 2</button><button>b 0</button><p>hello</p>`);
// 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, `<button>a 3</button><button>b 1</button><p>hello</p>`);
}
});

@ -0,0 +1,14 @@
<script>
let a = $state(0);
let b = $state(0);
let show = $state(true);
</script>
<button onclick={() => (a++, show = !show)}>a {a}</button>
<button onclick={() => (b++, show = !show)}>b {b}</button>
{#if show}
<p>hello</p>
{:else}
{await new Promise(() => {})}
{/if}

@ -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,
'<button>a_b 0_0</button> <button>b 0</button> <button>resolve</button> 0'
);
b.click();
await tick();
assert.htmlEqual(
target.innerHTML,
'<button>a_b 0_0</button> <button>b 0</button> <button>resolve</button> 0'
);
resolve.click();
await tick();
assert.htmlEqual(
target.innerHTML,
'<button>a_b 1_2</button> <button>b 2</button> <button>resolve</button> 1'
);
}
});

@ -0,0 +1,17 @@
<script>
let a = $state(0);
let b = $state(0);
let deferreds = [];
function push(value) {
if (!value) return value;
return new Promise(resolve => {
deferreds.push(() => resolve(value));
});
}
</script>
<button onclick={() => {a++;b++}}>a_b {a}_{b}</button>
<button onclick={() => (b++)}>b {b}</button>
<button onclick={() => (deferreds.shift()?.())}>resolve</button>
{await push(a)}

@ -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,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=6 values.length=1 values=[1]</p>
<div>not keyed:
<div>1</div>
</div>
<div>keyed:
<div>1</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=4 values.length=2 values=[1,2]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=2 values.length=3 values=[1,2,3]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=0 values.length=4 values=[1,2,3,4]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
`
);
add.click();
await tick();
add.click();
await tick();
add.click();
await tick();
add.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=8 values.length=4 values=[1,2,3,4]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
`
);
// pop should have no effect until earlier promises have also resolved
pop.click();
await tick();
pop.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=6 values.length=4 values=[1,2,3,4]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
`
);
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,
`
<button>add</button>
<button>shift</button>
<button>pop</button>
<p>pending=0 values.length=8 values=[1,2,3,4,5,6,7,8]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
<div>5</div>
<div>6</div>
<div>7</div>
<div>8</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
<div>5</div>
<div>6</div>
<div>7</div>
<div>8</div>
</div>
`
);
}
});

@ -11,18 +11,14 @@
return p.promise;
}
function shift() {
const fn = queue.shift();
if (fn) fn();
}
function addValue() {
values.push(values.length+1);
}
</script>
<button onclick={addValue}>add</button>
<button onclick={shift}>shift</button>
<button onclick={() => queue.shift()?.()}>shift</button>
<button onclick={() => queue.pop()?.()}>pop</button>
<p>
pending={$effect.pending()}
Loading…
Cancel
Save