From 1c6199fe487af533b50f9b9ebd2338d5e9f84be3 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 May 2024 13:48:53 +0100 Subject: [PATCH] fix: address derived memory leak on disconnection from reactive graph --- .changeset/afraid-worms-drum.md | 5 ++++ .../src/internal/client/reactivity/props.js | 1 + .../svelte/src/internal/client/runtime.js | 26 ++++++++++++++++--- packages/svelte/tests/signals/test.ts | 26 ++++++++++++++++++- 4 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 .changeset/afraid-worms-drum.md diff --git a/.changeset/afraid-worms-drum.md b/.changeset/afraid-worms-drum.md new file mode 100644 index 0000000000..ecbcd3b5b8 --- /dev/null +++ b/.changeset/afraid-worms-drum.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: address derived memory leak on disconnection from reactive graph diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 665d7fdc1f..2c2a8f3260 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -323,6 +323,7 @@ export function prop(props, key, flags, fallback) { from_child = true; set(inner_current_value, value); get(current_value); // force a synchronisation immediately + current_value.version++; } return value; diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index e70cbf3083..ffe321a656 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -204,6 +204,7 @@ export function check_dirtiness(reaction) { if (dependencies !== null) { var length = dependencies.length; var is_equal; + var reactions; for (var i = 0; i < length; i++) { var dependency = dependencies[i]; @@ -211,13 +212,13 @@ export function check_dirtiness(reaction) { if (!is_dirty && check_dirtiness(/** @type {import('#client').Derived} */ (dependency))) { is_equal = update_derived(/** @type {import('#client').Derived} **/ (dependency), true); } + var version = dependency.version; if (is_unowned) { // If we're working with an unowned derived signal, then we need to check // if our dependency write version is higher. If it is then we can assume // that state has changed to a newer version and thus this unowned signal // is also dirty. - var version = dependency.version; if (version > /** @type {import('#client').Derived} */ (reaction).version) { /** @type {import('#client').Derived} */ (reaction).version = version; @@ -228,7 +229,7 @@ export function check_dirtiness(reaction) { // If we are working with an unowned signal as part of an effect (due to !current_skip_reaction) // and the version hasn't changed, we still need to check that this reaction // if linked to the dependency source – otherwise future updates will not be caught. - var reactions = dependency.reactions; + reactions = dependency.reactions; if (reactions === null) { dependency.reactions = [reaction]; } else { @@ -238,6 +239,22 @@ export function check_dirtiness(reaction) { } else if ((reaction.f & DIRTY) !== 0) { // `signal` might now be dirty, as a result of calling `check_dirtiness` and/or `update_derived` return true; + } else { + // It might be that the derived was was dereferenced from its dependencies but has now come alive again. + // In thise case, we need to re-attach it to the graph and mark it dirty if any of its dependencies have + // changed since. + if (version > /** @type {import('#client').Derived} */ (reaction).version) { + /** @type {import('#client').Derived} */ (reaction).version = version; + is_dirty = true; + } + reactions = dependency.reactions; + if (reactions === null || !reactions.includes(reaction)) { + if (reactions === null) { + dependency.reactions = [reaction]; + } else { + reactions.push(reaction); + } + } } } } @@ -422,7 +439,8 @@ function remove_reaction(signal, dependency) { } } } - if (reactions_length === 0 && (dependency.f & UNOWNED) !== 0) { + debugger; + if (reactions_length === 0 && (dependency.f & DERIVED) !== 0) { // If the signal is unowned then we need to make sure to change it to maybe dirty. set_signal_status(dependency, MAYBE_DIRTY); remove_reactions(/** @type {import('#client').Derived} **/ (dependency), 0); @@ -815,7 +833,7 @@ export function get(signal) { ) { if (current_dependencies === null) { current_dependencies = [signal]; - } else { + } else if (current_dependencies[current_dependencies.length - 1] !== signal) { current_dependencies.push(signal); } } diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 215f39e67b..0bf85a6f87 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -2,13 +2,14 @@ import { describe, assert, it } from 'vitest'; import { flushSync } from '../../src/index-client'; import * as $ from '../../src/internal/client/runtime'; import { + destroy_effect, effect, effect_root, render_effect, user_effect } from '../../src/internal/client/reactivity/effects'; import { source, set } from '../../src/internal/client/reactivity/sources'; -import type { Derived, Value } from '../../src/internal/client/types'; +import type { Derived, Effect, Value } from '../../src/internal/client/types'; import { proxy } from '../../src/internal/client/proxy'; import { derived } from '../../src/internal/client/reactivity/deriveds'; @@ -423,4 +424,27 @@ describe('signals', () => { destroy(); }; }); + + test('owned deriveds correctly cleanup when no longer connected to graph', () => { + let a: Derived; + let state = source(0); + + const destroy = effect_root(() => { + render_effect(() => { + a = derived(() => { + $.get(state); + }); + $.get(a); + }); + }); + + return () => { + flushSync(); + assert.equal(a?.deps?.length, 1); + assert.equal(state?.reactions?.length, 1); + destroy(); + assert.equal(a?.deps?.length, 1); + assert.equal(state?.reactions, null); + }; + }); });