fix: prevent batch unlinking twice (#18303)

Merged #18298 a bit too soon - there's still a situation which can
corrput the linked list: If we merge one batch into another, it will
unlink the batch immediately. But `discard` also calls it, running the
unlink logic again, which can corrupt the linked list.

Sketch:

1. Batch A is pending.
2. Batch B starts later, intersects A, resolves first, and merges into
A.
3. B is unlinked immediately, but `A.oncommit(() => B.discard())`
remains.
4. Independent batch C is created while A is still pending and is linked
after A.
5. A commits, calls `B.discard()`, and B's stale `#prev`/`#next` can set
`A.#next = null` / `last_batch = A`, disconnecting C.

Not able to produce a failing test from it but it's definitely a fix we
need to make.

Also moved `#link` into a constructor, because it is (and should be)
used only once.

Also made the action after an error `discard` instead of just `#unlink`
because this batch is done for and e.g. pending `settled` should
resolve, too.
pull/18311/head
Simon H 3 months ago committed by GitHub
parent 0da9f9e2ab
commit 638ab370a5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -214,6 +214,18 @@ export class Batch {
#decrement_queued = false;
constructor() {
// link batch
if (last_batch === null) {
first_batch = last_batch = this;
} else {
last_batch.#next = this;
this.#prev = last_batch;
}
last_batch = this;
}
#is_deferred() {
if (this.is_fork) return true;
@ -327,11 +339,11 @@ export class Batch {
} catch (e) {
reset_all(root);
// If there's no async work left, this branch is now dead and needs
// to be unlinked to not become a zombie that is never cleaned up.
// to be discarded to not become a zombie that is never cleaned up.
// See https://github.com/sveltejs/svelte/issues/18221#issuecomment-4497918414
// for a (non-minimal) reproduction that demonstrates a case where this is necessary
// to not get follow-up false-positives via "batch has scheduled roots" invariant errors.
if (!this.#is_deferred()) this.#unlink();
if (!this.#is_deferred()) this.discard();
throw e;
}
}
@ -852,7 +864,6 @@ export class Batch {
static ensure() {
if (current_batch === null) {
const batch = (current_batch = new Batch());
batch.#link();
if (!is_processing && !is_flushing_sync) {
queue_micro_task(() => {
@ -972,18 +983,11 @@ export class Batch {
this.#roots.push(e);
}
#link() {
if (last_batch === null) {
first_batch = last_batch = this;
} else {
last_batch.#next = this;
this.#prev = last_batch;
}
last_batch = this;
}
#unlink() {
// #merge calls #unlink, discard later on does it again - prevent
// running it multiple times to not corrupt the linked list
if (!this.linked) return;
var prev = this.#prev;
var next = this.#next;

Loading…
Cancel
Save