Edit: see comments below for real issue (memory leak) <p dir="aut

Thanks for the amazing work, <a class="user-mention notranslate" data-hovercard-type="

I filed a Chrome bug for this here: <a href="https://bugs.chromium.org/p/chromium/issu

Proxies permanently leak memory about comlink HOT 12 CLOSED

googlechromelabs commented on September 26, 2024 1

Proxies permanently leak memory

from comlink.

Comments (12)

surma commented on September 26, 2024 4

Thanks for the amazing work, @AshleyScirra. I am talking to Chromies as well, so let's all continue in the bug you opened.

Leaving this issue open until we have come to a conclusion/fix.

from comlink.

surma commented on September 26, 2024 4

I implemented usage of the WeakRef proposal on a branch that takes care of propagating GC’ing of proxies to the worker. If you fancy, take a look: #469

from comlink.

AshleyScirra commented on September 26, 2024 3

Won't the GC pin the proxy in memory because of the message handler? Or does Chrome specifically handle that and it knows when the other end of the MessagePort is also GC'd? I might run some tests on this to see how it works out.

from comlink.

AshleyScirra commented on September 26, 2024 2

OK, so out of interest, how is that collected? When I drop the reference to the returned object on the DOM, how does it know to collect the real object on the worker side?

from comlink.

AshleyScirra commented on September 26, 2024 1

OK, so here's a proof of concept that Comlink permanently leaks memory:

<!-- index.html -->
<!doctype html>
<script src="comlink.js"></script>
<script>
"use strict";
{
	const worker = new Worker("worker.js");
	const api = Comlink.proxy(worker);

	async function test()
	{
		const app = await new api.App();
		const returned = await app.getObject();
		await returned.work();
	}
	
	function stress()
	{
		const promises = [];
		
		for (let i = 0; i < 1000; ++i)
			promises.push(test());
		
		Promise.all(promises).then(setTimeout(stress, 20));
	}
	
	stress();
}
</script>

// worker.js
"use strict";

importScripts("comlink.js");

let dummy = 0;

class ReturnedObject {
	work()
	{
		++dummy;
	}
}

class App {
	getObject()
	{
		return Comlink.proxyValue(new ReturnedObject());
	}
}

Comlink.expose({App, ReturnedObject}, self);

Showing Chrome's task manager while running this, and repeatedly clicking "collect garbage" in dev tools, still shows permanently increasing memory usage.

I mentioned the MessageChannel approach on the async-dom email list ([email protected]) and a Google engineer mentioned that currently ports live as long as their frame, which would explain the memory leak. Allowing collection of unreferenced MessageChannels should fix this leak, and then should also provide a mechanism for via.js to work. (I think this approach will have a painfully high performance overhead, but right now AFAIK there's no way to do it without leaking memory.)

from comlink.

AshleyScirra commented on September 26, 2024 1

I filed a Chrome bug for this here: https://bugs.chromium.org/p/chromium/issues/detail?id=798855

However this probably needs to be tested across other major browsers and similar issues filed if they are also affected.

from comlink.

surma commented on September 26, 2024

Function parameters and function return values are structurally cloned by default. If you want them to be proxy’d as well, wrap them in Comlink.proxyValue(). In your case:

class App {
  getObject() {
    return Comlink.proxyValue(new ReturnedObject());
  }
}

from comlink.

surma commented on September 26, 2024

Every proxied value (and when you use new with a Comlink proxy) will create a new, dedicated message channel pair. So the actual value won’t be garbage collected until the new proxy is GC’d.

Proxy -> MessagePort A -> Worker -> MessagePort B -> Proxy’d value

Does that explain it?

from comlink.

mhofman commented on September 26, 2024

I believe that even if the MessagePort were closed when the remote is garbage collected, this approach would still leak memory in the case of cyclical references across the 2 contexts.

This will probably require deeper changes in the JavaScript engine.
I wrote a proposal based on WeakRef to allow such a use case: tc39/proposal-weakrefs#65

from comlink.

AshleyScirra commented on September 26, 2024

WeakRefs themselves are enough to implement ComLink without creating memory leaks. I wrote a proof-of-concept of this in via.js. There is no need to use leaky MessageChannels, and therefore that problem should not need solving.

from comlink.

mhofman commented on September 26, 2024

@AshleyScirra, that is actually not accurate.

via.js does not support 2-way proxying of objects, so there is simply no way of creating circular references with the current API. Only objects from the receiver are wrapped. Something like this is not possible:

// In the controller
var delegate = new Delegate();
via.registerDelegate(delegate);

If you read the problem statement in my proposal, you'll actually see that it assumes a mechanism similar to via.js, where handles for objects are created, and commands are sent across referencing the handles.
However it assumes uses a 2-way proxying system where each side is symmetrical, and complex types will be automatically wrapped for use on the other side.

My solution is really meant to be a general way of handling garbage collection across different realms.
Ultimately I believe browsers should be able to implement abstractions on top these low level GC primitives that don't expose the JavaScript code to the GC state itself. I have a proposal for that to follow.

from comlink.

surma commented on September 26, 2024

This is now fixed (kinda) via #348. I'll revisit this once we have WeakRef :)

from comlink.

Proxies permanently leak memory about comlink HOT 12 CLOSED

Comments (12)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent