Callbacks preventing garbage collection of objects #223
Comments
|
It's worth noting that calling the exported From what I can see, cleanup comes from RTCWrapper which closes all currently open Give that you can't re-open a RTCPeerConnection that's been closed, it seems reasonable to not expect any events to fire after it's been closed, so resetting all the callbacks on close (instead of just destroy) would be ok? |
|
Destroying or resetting callbacks will do the same. But the point is that; if you call destroy or reset callbacks then you will not receive closed state change, which is not good. |
|
Calling If I run this script with the import { RTCPeerConnection } from 'node-datachannel/polyfill'
while (true) {
let conn = new RTCPeerConnection()
conn.close()
conn = null
}The WebRTC spec says that calling close on a RTCPeerConnection tears down any associated resources, so it should become garbage collectable after that. If it doesn't then this is a memory leak. |
|
Hello, If GC is running then it should call the destructor and reset all callbacks etc. Could you please check and try this? |
|
With #224 applied the above script still runs out of memory after a minute or so. Interestingly I was trying to make it be kinder to the garbage collector by waiting for the connection to close before creating a new one with: while (true) {
const conn = new RTCPeerConnection()
conn.close()
await new Promise(resolve => {
conn.addEventListener('connectionstatechange', () => {
if (conn.connectionState === 'closed') {
resolve()
}
})
})
}with this it runs for a few iterations then the process exits with code 13 and no output: % node index.js
% echo $?
13If I only create a single RTCPeerConnection #224 does let the process exit, though it takes a long time: import { RTCPeerConnection } from 'node-datachannel/polyfill'
const conn = new RTCPeerConnection()
conn.addEventListener('connectionstatechange', () => {
if (conn.connectionState === 'closed') {
console.timeEnd('close')
}
})
console.time('exit')
console.time('close')
conn.close()
process.addListener('exit', () => {
console.timeEnd('exit')
process.exit(0)
})8 seconds to close! % node index.js
close: 0.449ms
exit: 8.100sApplying this diff to #224 makes the closing almost instant: diff --git a/polyfill/RTCPeerConnection.js b/polyfill/RTCPeerConnection.js
index db3ae16..171c10e 100644
--- a/polyfill/RTCPeerConnection.js
+++ b/polyfill/RTCPeerConnection.js
@@ -61,6 +61,11 @@ export default class _RTCPeerConnection extends EventTarget {
// forward peerConnection events
this.#peerConnection.onStateChange(() => {
+ if (this.connectionState === 'closed') {
+ this.#peerConnection.destroy();
+ this.#peerConnection = null;
+ }
+
this.dispatchEvent(new Event('connectionstatechange'));
});
@@ -227,7 +232,6 @@ export default class _RTCPeerConnection extends EventTarget {
});
this.#peerConnection?.close();
- this.#peerConnection = null;
}
createAnswer() {E.g. instead of % node index.js
close: 0.436ms
exit: 5.802ms5ms to close, much faster. As an alternative to #224, this diff applied against master also solves the problem: diff --git a/polyfill/RTCPeerConnection.js b/polyfill/RTCPeerConnection.js
index be9913b..08accf1 100644
--- a/polyfill/RTCPeerConnection.js
+++ b/polyfill/RTCPeerConnection.js
@@ -61,6 +61,10 @@ export default class _RTCPeerConnection extends EventTarget {
// forward peerConnection events
this.#peerConnection.onStateChange(() => {
+ if (this.connectionState === 'closed') {
+ this.#peerConnection.destroy();
+ }
+
this.dispatchEvent(new Event('connectionstatechange'));
});Setting Really I think the root cause is the use of |
|
@achingbrain Thanks for the detailed investigation. To summarize things;
According to this info, we need to select an option. |
|
I tried with Chrome. If you call the close function of a peer-connection, the peer does not receive any closed event. The other peer receives a disconnected event. So it seems we can call the destroy function directly within the close function. |
|
Indeed, with the Web API, |
|
I have just published v0.5.5 |
|
Sorry for the delay in replying, I've been AFK for a little while. If I run: import { RTCPeerConnection } from 'node-datachannel/polyfill'
const conn = new RTCPeerConnection()
conn.addEventListener('connectionstatechange', () => {
if (conn.connectionState === 'closed') {
console.timeEnd('close')
}
})
console.time('exit')
console.time('close')
conn.close()
process.addListener('exit', () => {
console.timeEnd('exit')
process.exit(0)
})The process now exits almost immediately, so the initially reported issue is fixed, thanks for pushing that release. 🎉 🚀 However this script still causes the process to run out of memory after a minute or so: import { RTCPeerConnection } from 'node-datachannel/polyfill'
while (true) {
const conn = new RTCPeerConnection()
conn.close()
}...and this script causes the process to terminate unexpectedly with a non-zero error code (it should run forever due to no import { RTCPeerConnection } from 'node-datachannel/polyfill'
while (true) {
const conn = new RTCPeerConnection()
conn.close()
await new Promise(resolve => {
conn.addEventListener('connectionstatechange', () => {
if (conn.connectionState === 'closed') {
resolve()
}
})
})
} |
Run Result Same result as @achingbrain for |
|
I have just released v0.9.0 Should we close also this issue? |
|
I am closing the issue. Please feel free to open if you need to. |
This is a follow-on to #215, I think the problem still exists, even after
[email protected].These references can keep threads alive:
The referenced lines in
RTCPeerConnectionare all callbacks registered with the C++ object, for example line 63:node-datachannel/polyfill/RTCPeerConnection.js
Line 63 in d866015
If I change the
.closemethod ofRTCPeerConnectionthe problem goes away:I think this is because doDestroy calls doResetCallbacks which releases references to the JS callbacks.
To replicate, clone js-libp2p, apply this diff:
Then install & build and run the node tests:
You should see one test run, a brief pause then a list of all the handles keeping the process running:
Ignore the
FILEHANDLEandKEYPAIRGENREQUESTentries - it's theThreadSafeCallback callbackentries causing the problem.The text was updated successfully, but these errors were encountered: