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: remove memory leak from bind:this #11194

Merged
merged 9 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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/itchy-eels-marry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: remove memory leak from bind:this
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { STATE_SYMBOL } from '../../../constants.js';
import { effect, render_effect } from '../../../reactivity/effects.js';
import { untrack } from '../../../runtime.js';
import { queue_task } from '../../task.js';

/**
* @param {any} bound_value
Expand Down Expand Up @@ -47,7 +48,8 @@ export function bind_this(element_or_component, update, get_value, get_parts) {
});

return () => {
effect(() => {
// We cannot use effects in the teardown phase, we we use a microtask instead.
queue_task(() => {
if (parts && is_bound_this(get_value(...parts), element_or_component)) {
update(null, ...parts);
}
Expand Down
20 changes: 9 additions & 11 deletions packages/svelte/src/internal/client/dom/task.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import { run_all } from '../../shared/utils.js';

let is_task_queued = false;
let is_raf_queued = false;

/** @type {Array<() => void>} */
let current_queued_tasks = [];
/** @type {Array<() => void>} */
let current_raf_tasks = [];

function process_task() {
is_task_queued = false;
Expand All @@ -15,11 +12,15 @@ function process_task() {
run_all(tasks);
}

function process_raf_task() {
is_raf_queued = false;
const tasks = current_raf_tasks.slice();
current_raf_tasks = [];
run_all(tasks);
/**
* @param {() => void} fn
*/
export function queue_task(fn) {
if (!is_task_queued) {
is_task_queued = true;
queueMicrotask(process_task);
}
current_queued_tasks.push(fn);
}

/**
Expand All @@ -29,7 +30,4 @@ export function flush_tasks() {
if (is_task_queued) {
process_task();
}
if (is_raf_queued) {
process_raf_task();
}
}
34 changes: 33 additions & 1 deletion packages/svelte/src/internal/client/reactivity/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import {
destroy_effect_children,
execute_effect,
get,
is_destroying_effect,
is_flushing_effect,
remove_reactions,
schedule_effect,
set_is_destroying_effect,
set_is_flushing_effect,
set_signal_status,
untrack
Expand Down Expand Up @@ -109,6 +111,12 @@ export function user_effect(fn) {
(DEV ? ': The Svelte $effect rune can only be used during component initialisation.' : '')
);
}
if (is_destroying_effect) {
throw new Error(
'ERR_SVELTE_EFFECT_IN_TEARDOWN' +
(DEV ? ': The Svelte $effect rune can not be used in the teardown phase of an effect.' : '')
);
}

// Non-nested `$effect(...)` in a component should be deferred
// until the component is mounted
Expand Down Expand Up @@ -140,6 +148,14 @@ export function user_pre_effect(fn) {
: '')
);
}
if (is_destroying_effect) {
throw new Error(
'ERR_SVELTE_EFFECT_IN_TEARDOWN' +
(DEV
? ': The Svelte $effect.pre rune can not be used in the teardown phase of an effect.'
: '')
);
}

return render_effect(fn);
}
Expand Down Expand Up @@ -228,6 +244,22 @@ export function branch(fn) {
return create_effect(RENDER_EFFECT | BRANCH_EFFECT, fn, true);
}

/**
* @param {import("#client").Effect} effect
*/
export function execute_effect_teardown(effect) {
var teardown = effect.teardown;
if (teardown !== null) {
const previously_destroying_effect = is_destroying_effect;
set_is_destroying_effect(true);
try {
teardown.call(null);
} finally {
set_is_destroying_effect(previously_destroying_effect);
}
}
}

/**
* @param {import('#client').Effect} effect
* @returns {void}
Expand All @@ -249,7 +281,7 @@ export function destroy_effect(effect) {
}
}

effect.teardown?.call(null);
execute_effect_teardown(effect);

var parent = effect.parent;

Expand Down
17 changes: 14 additions & 3 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ import {
object_prototype
} from './utils.js';
import { snapshot } from './proxy.js';
import { destroy_effect, effect, user_pre_effect } from './reactivity/effects.js';
import {
destroy_effect,
effect,
execute_effect_teardown,
user_pre_effect
} from './reactivity/effects.js';
import {
EFFECT,
RENDER_EFFECT,
Expand Down Expand Up @@ -37,12 +42,18 @@ let current_scheduler_mode = FLUSH_MICROTASK;
// Used for handling scheduling
let is_micro_task_queued = false;
export let is_flushing_effect = false;
export let is_destroying_effect = false;

/** @param {boolean} value */
export function set_is_flushing_effect(value) {
is_flushing_effect = value;
}

/** @param {boolean} value */
export function set_is_destroying_effect(value) {
is_destroying_effect = value;
}

// Used for $inspect
export let is_batching_effect = false;
let is_inspecting_signal = false;
Expand Down Expand Up @@ -406,7 +417,7 @@ export function execute_effect(effect) {
destroy_effect_children(effect);
}

effect.teardown?.call(null);
execute_effect_teardown(effect);
var teardown = execute_reaction_fn(effect);
effect.teardown = typeof teardown === 'function' ? teardown : null;
} finally {
Expand Down Expand Up @@ -658,11 +669,11 @@ export function flush_sync(fn, flush_previous = true) {

var result = fn?.();

flush_tasks();
if (current_queued_root_effects.length > 0 || root_effects.length > 0) {
flush_sync();
}

flush_tasks();
flush_count = 0;

return result;
Expand Down
Loading