Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/major-beans-fry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: unset context on stale promises
22 changes: 18 additions & 4 deletions packages/svelte/src/internal/client/reactivity/deriveds.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,20 +113,30 @@ export function async_derived(fn, location) {
// only suspend in async deriveds created on initialisation
var should_suspend = !active_reaction;

/** @type {Map<Batch, ReturnType<typeof deferred<V>>>} */
/** @type {Map<Batch, ReturnType<typeof deferred<V>> & { rejected?: boolean }>} */
var deferreds = new Map();

async_effect(() => {
if (DEV) current_async_effect = active_effect;

/** @type {ReturnType<typeof deferred<V>>} */
/** @type {ReturnType<typeof deferred<V>> & { rejected?: boolean }} */
var d = deferred();
promise = d.promise;

try {
// If this code is changed at some point, make sure to still access the then property
// of fn() to read any signals it might access, so that we track them as dependencies.
Promise.resolve(fn()).then(d.resolve, d.reject);
Promise.resolve(fn()).then((v) => {
if (d.rejected) {
// If we rejected this stale promise, d.resolve
// is a noop (d.promise.then(handler) below will never run).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're not doing d.promise.then(handler), we're doing d.promise.then(onresolved, onrejected) — I'd expect onrejected (which does call unset_context) to run if d.promise rejects. what am I missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same question, glad it didn't trick only me. The order of events is

  1. async_effect creates the promise, and run fn which saves the context
  2. Something changes, async_effect creates a new promise and reject the old one, handler is executed with null and the context is cleared
  3. This new promise resolve, the context is restored by the return value of save
  4. First promise resolves the context is restored, d.resolve is invoked and that should clear the context but since d was already rejected it does nothing

The context is never cleared and there's a hanging active_reaction

Copy link
Member Author

@dummdidumm dummdidumm Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of events is this:

  1. you enter X, batch runs
  2. you enter Y, X hasn't resolve yet, batch runs
  3. Y resolves before X, X is stale -> you reject the promise for X -> d.promise.then below runs, also unsets context but that doesn't matter, because ...
  4. some time later X resolves. The generated code looks something like $.async_derived((await save(x())())), which means the restore method of save runs. That means the context is now set again. But d.promise.then(handle) will NOT run, because that promise is already done (we called its reject), so the context is never unset (unless with my fix)

Here's a playground that shows you the error without the fix (type in 1234 fast enough): https://svelte.dev/playground/hello-world?version=5.39.11#H4sIAAAAAAAACm1RwWrjMBD9lWEoRGq9bnroxU0Kve1xWXpb7UG1J4lAGRlpbKcY__uiaNO00ItA781783gzI9sjYYM_yfsAU4i-A0WdE-o0VrhznhI2f2aU9z7PZQCri-ql7-s0kpeMvdlE3-FtYCGWhA1uUhtdL8-GjXgSaMPAAlu4SWKF1Fo_XZjR-oGuzGqlDWduN3ArLjDY1KlRw5zBIomwhV8xHF2ienJy-E0p-JFiUsXWSBs4BU-1D3u1SmKjON6vKhh14cvLNF18VITt82WHkUTy6o4UBlGxAvX4Y6w98V4OGm7hYb1e__dZdC0HYqX0F_nn9QbjOZ7jvcGc4Ok6NbDc3X38Y10mSY0f9p_jRpIhMsS6L5nPwqW0lTcKnHKPHUU3UqfsZJ2U9nLFWhve3F_PwhvH_SDw5rhryhHuz_h8zrUYnk8LVih0EmwkDrT8rVCs85PjDpud9YmWf-ytupNXAgAA

I don't know why my test does not show the error in the playground, I thought it did.
I didn't get this to show up with button clicks yet, maybe because our event handlers unset the context before flushing, masking the error.

As to monkey-patching: We can just do let d = { ...Promise.withResolvers(), rejected: false } later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Couldn't we just do it like this though? #16936

// In this case we need to unset the restored context here
// to avoid leaking it (and e.g. cause false-positive mutation errors).
unset_context();
} else {
d.resolve(v);
}
}, d.reject);
} catch (error) {
d.reject(error);
}
Expand All @@ -141,7 +151,11 @@ export function async_derived(fn, location) {
if (!pending) {
batch.increment();

deferreds.get(batch)?.reject(STALE_REACTION);
var previous_deferred = deferreds.get(batch);
if (previous_deferred) {
previous_deferred.rejected = true;
previous_deferred.reject(STALE_REACTION);
}
deferreds.set(batch, d);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { test } from '../../test';

export default test({
async test({ assert, target }) {
// We gotta wait a bit more in this test because of the macrotasks in App.svelte
function macrotask(t = 3) {
return new Promise((r) => setTimeout(r, t));
}

await macrotask();
assert.htmlEqual(target.innerHTML, '<input> 1 | ');

const [input] = target.querySelectorAll('input');

input.value = '1';
input.dispatchEvent(new Event('input', { bubbles: true }));
await macrotask();
assert.htmlEqual(target.innerHTML, '<input> 1 | ');

input.value = '12';
input.dispatchEvent(new Event('input', { bubbles: true }));
await macrotask(6);
// TODO this is wrong (separate bug), this should be 3 | 12
assert.htmlEqual(target.innerHTML, '<input> 5 | 12');
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<script>
let count = $state(0);
let value = $state('');
let prev;

function asd(v) {
const r = Promise.withResolvers();

if (prev || v === '') {
console.log('hello', !!prev)
Promise.resolve().then(async () => {
console.log('count++')
count++;
r.resolve(v);
await new Promise(r => setTimeout(r, 0));
// TODO with a microtask like below it still throws a mutation error
// await Promise.resolve();
prev?.resolve();
})
} else {
console.log('other')
prev = Promise.withResolvers();
prev.promise.then(() => {
console.log('other coun++')
count++;
r.resolve(v)
})
}

return r.promise;
}

const x = $derived(await asd(value))
</script>

<input bind:value />

{count} | {x}
Loading