-
Notifications
You must be signed in to change notification settings - Fork 236
Conversation
Changeset ✓This PR includes a changeset covering all affected packages:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Devin Review: No Issues Found
Devin Review analyzed this PR and found no bugs or issues to report.
raparanjpe
commented
Sep 4, 2026
Hi @alan-george-lk! Thank you for proposing this fix; I believe I followed a similar approach to weakly capturing the RoomSession in #1377. For the data channel I was going to follow a similar approach to you here, but noticed we can avoid capturing the data channel in its own closure altogether by changing the interface slightly. Feel free to use that implementation if it's easier than manually unsetting the closures.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
33891b7 to
d539b63
Compare
...t-sdk-rust into alan/bugfix-room-lifecycle
...it/client-sdk-rust into alan/bugfix-room-lifecycle
...it/client-sdk-rust into alan/bugfix-room-lifecycle
e99174b to
7c3310a
Compare
...it/client-sdk-rust into alan/bugfix-room-lifecycle
Hi @alan-george-lk! Thank you for proposing this fix; I believe I followed a similar approach to weakly capturing the RoomSession in #1377. For the data channel I was going to follow a similar approach to you here, but noticed we can avoid capturing the data channel in its own closure altogether by changing the interface slightly. Feel free to use that implementation if it's easier than manually unsetting the closures.
Hey @raparanjpe, great minds think alike -- I just reviewed your PR. I think removing amount is cleaner, but there are test items/a PR stack here addressing a few related changes.
(削除) Can you retarget your PR to this one in the next day or two, keeping my changes except for the data channel related ones (taking your approach instead)? Then I can re-test everything with a memory tracker util I have on a separate SDK that brings in these changes. (削除ここまで)
I have pulled in your commit such that you get credit. We may merge this before you have a chance to review, but I'll make sure your name is in the author credits.
(cherry picked from commit 4c3b857)
...it/client-sdk-rust into alan/bugfix-room-lifecycle
...it/client-sdk-rust into alan/bugfix-room-lifecycle
...it/client-sdk-rust into alan/bugfix-room-lifecycle
raparanjpe
commented
Sep 8, 2026
@alan-george-lk no problem at all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to set these to None anymore as the closure isn't capturing the arc reference on these data channels!
Uh oh!
There was an error while loading. Please reload this page.
Summary
RoomSessionownership cycle by capturing it weakly from the E2EE callbackMemory impact
Previously, repeated room joins retained room sessions, SDP/codec allocations, peer connections, and WebRTC threads. After these fixes, 1,000 connect/disconnect cycles dropped from approximately 488 MiB RSS to 44 MiB, with stable thread and file-descriptor counts.
Testing