Fix memory leaks and race conditions from improper timeout management#2951
Fix memory leaks and race conditions from improper timeout management#2951saghul merged 2 commits intojitsi:masterfrom
Conversation
- Fix critical 5-second timeout leak in ChatRoom.leave() where timeout was never cleared when MUC_LEFT fired successfully - Add comprehensive dispose() method to QualityController to clear all active timeouts (_timer, _limitedByCpuTimeout, _lastNRampupTimeout) - Fix orphaned 5-second keepalive timeout in XmppConnection by storing timeout reference - Add QualityController disposal to JitsiConference.leave() lifecycle - Prevent zombie timers, memory leaks, and race conditions in long-running conferences Fixes jitsi#2949
|
Hi, thanks for your contribution! |
modules/xmpp/ChatRoom.ts
Outdated
| clearTimeout(timeout as number); | ||
|
|
||
| // Always clear the timeout to prevent memory leaks and race conditions | ||
| if (timeout !== undefined) { |
There was a problem hiding this comment.
You don't need to do this, this part was working ok before, clearTimeout is resilient to bad input.
There was a problem hiding this comment.
ok I have removed the changes.
| // and to allow backend to correct any possible split brain issues | ||
| setTimeout(() => this._keepAliveAndCheckShard(), 5000); | ||
| // Store timeout so it can be cleared if needed | ||
| this._wsKeepAlive = setTimeout(() => this._keepAliveAndCheckShard(), 5000); |
There was a problem hiding this comment.
So there are 3 instances -
-
around line 423 - When the status becomes DISCONNECTED, it calls
clearTimeout(this._wsKeepAlive)to clean up before attempting to resume the connection. -
line 469 - The code clears the timeout as part of disconnecting the XMPP connection.
-
around line 537 -
_maybeStartWSKeepAliveclears any existing timeout before scheduling a new keep-alive interval.
| } | ||
|
|
||
| if (this.qualityController) { | ||
| this.qualityController.dispose(); |
|
@saghul I have done the necessary changes. Could you check it again? |
|
Jenkins please test this please. |
3 participants
Fixes #2949