Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: addresses memory leak when creating deriveds inside untrack #14443

Merged
merged 11 commits into from
Nov 26, 2024
5 changes: 5 additions & 0 deletions .changeset/great-crabs-rhyme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: addresses memory leak when creating deriveds inside untrack
trueadm marked this conversation as resolved.
Show resolved Hide resolved
42 changes: 30 additions & 12 deletions packages/svelte/src/internal/client/reactivity/deriveds.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ export function derived(fn) {
active_effect.f |= EFFECT_HAS_DERIVED;
}

var parent_derived =
active_reaction !== null && (active_reaction.f & DERIVED) !== 0
? /** @type {Derived} */ (active_reaction)
: null;

/** @type {Derived<V>} */
const signal = {
children: null,
Expand All @@ -53,12 +58,11 @@ export function derived(fn) {
reactions: null,
v: /** @type {V} */ (null),
version: 0,
parent: active_effect
parent: parent_derived || active_effect
trueadm marked this conversation as resolved.
Show resolved Hide resolved
};

if (active_reaction !== null && (active_reaction.f & DERIVED) !== 0) {
var derived = /** @type {Derived} */ (active_reaction);
(derived.children ??= []).push(signal);
if (parent_derived !== null) {
(parent_derived.children ??= []).push(signal);
}

return signal;
Expand Down Expand Up @@ -104,6 +108,21 @@ function destroy_derived_children(derived) {
*/
let stack = [];

/**
* @param {Derived} derived
* @returns {Effect | null}
*/
function get_derived_parent_effect(derived) {
var parent = derived.parent;
while (parent !== null) {
if ((parent.f & DERIVED) === 0) {
return /** @type {Effect} */ (parent);
}
parent = parent.parent;
}
return null;
}

/**
* @template T
* @param {Derived} derived
Expand All @@ -113,7 +132,7 @@ export function execute_derived(derived) {
var value;
var prev_active_effect = active_effect;

set_active_effect(derived.parent);
set_active_effect(get_derived_parent_effect(derived));

if (DEV) {
let prev_inspect_effects = inspect_effects;
Expand Down Expand Up @@ -162,14 +181,13 @@ export function update_derived(derived) {
}

/**
* @param {Derived} signal
* @param {Derived} derived
* @returns {void}
*/
export function destroy_derived(signal) {
destroy_derived_children(signal);
remove_reactions(signal, 0);
set_signal_status(signal, DESTROYED);
export function destroy_derived(derived) {
destroy_derived_children(derived);
remove_reactions(derived, 0);
set_signal_status(derived, DESTROYED);

// TODO we need to ensure we remove the derived from any parent derives
signal.v = signal.children = signal.deps = signal.ctx = signal.reactions = null;
derived.v = derived.children = derived.deps = derived.ctx = derived.reactions = null;
}
5 changes: 4 additions & 1 deletion packages/svelte/src/internal/client/reactivity/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,15 @@ export interface Reaction extends Signal {
fn: null | Function;
/** Signals that this signal reads from */
deps: null | Value[];
parent: Effect | null;
}

export interface Derived<V = unknown> extends Value<V>, Reaction {
/** The derived function */
fn: () => V;
/** Reactions created inside this signal */
children: null | Reaction[];
/** Parent effect or derived */
parent: Effect | Derived | null;
}

export interface Effect extends Reaction {
Expand Down Expand Up @@ -58,6 +59,8 @@ export interface Effect extends Reaction {
first: null | Effect;
/** Last child effect created inside this signal */
last: null | Effect;
/** Parent effect */
parent: Effect | null;
/** Dev only */
component_function?: any;
}
Expand Down
19 changes: 17 additions & 2 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -767,9 +767,24 @@ export function get(signal) {
} else if (is_derived && /** @type {Derived} */ (signal).deps === null) {
var derived = /** @type {Derived} */ (signal);
var parent = derived.parent;
var target = derived;

if (parent !== null && !parent.deriveds?.includes(derived)) {
(parent.deriveds ??= []).push(derived);
while (parent !== null) {
// Attach the derived to the nearest parent effect, if there are deriveds
// in between then we also need to attach them too
if ((parent.f & DERIVED) !== 0) {
var parent_derived = /** @type {Derived} */ (parent);

target = parent_derived;
parent = parent_derived.parent;
} else {
var parent_effect = /** @type {Effect} */ (parent);

if (!parent_effect.deriveds?.includes(target)) {
(parent_effect.deriveds ??= []).push(target);
}
break;
}
}
}

Expand Down
26 changes: 26 additions & 0 deletions packages/svelte/tests/signals/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -739,4 +739,30 @@ describe('signals', () => {
assert.deepEqual(a.reactions, null);
};
});

test('nested deriveds clean up the releationships when used with untrack', () => {
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
return () => {
let a = render_effect(() => {});

const destroy = effect_root(() => {
a = render_effect(() => {
$.untrack(() => {
const b = derived(() => {
const c = derived(() => {});
$.untrack(() => {
$.get(c);
});
});
$.get(b);
});
});
});

assert.deepEqual(a.deriveds?.length, 1);

destroy();

assert.deepEqual(a.deriveds, null);
};
});
});