CudaContext lifecycle memory fixes - #1395
Conversation
Changeset ✓This PR includes a changeset covering all affected packages:
|
| // impls can call drop_handle without re-entering DashMap::clear(). | ||
| *self.config.lock() = None; | ||
| self.handle_dropped_txs.clear(); | ||
| let leftover: Vec<_> = self.ffi_handles.iter().map(|entry| *entry.key()).collect(); |
There was a problem hiding this comment.
Rust novice question: is this the cleanest way to grab remaining handles before iterating? Feels a bit complex
There was a problem hiding this comment.
This works, but you should be able to avoid the call to collect (which allocates) and just consume the iterator directly using the for loop.
There was a problem hiding this comment.
Appreciate the suggestion, although I changed it and the test that covers this now deadlocks. I'll keep the original code since it seems to be stable.
| void CudaContext::Shutdown() { | ||
| // Shutdown CUDA context | ||
| std::lock_guard<std::mutex> lock(cudaMutex()); | ||
| if (ref_count_ == 0) { |
There was a problem hiding this comment.
The refcount kind of sucks, but is needed because both the encoder/decoder factories reference this.
| // impls can call drop_handle without re-entering DashMap::clear(). | ||
| *self.config.lock() = None; | ||
| self.handle_dropped_txs.clear(); | ||
| let leftover: Vec<_> = self.ffi_handles.iter().map(|entry| *entry.key()).collect(); |
There was a problem hiding this comment.
This works, but you should be able to avoid the call to collect (which allocates) and just consume the iterator directly using the for loop.
alan-george-lk
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm, one question
| let existed = self.ffi_handles.remove(&id).is_some(); | ||
| self.handle_dropped_txs.remove(&id); | ||
| if !existed { | ||
| if !existed && self.is_setup() { |
There was a problem hiding this comment.
curiously, why self.is_setup() is needed here?
There was a problem hiding this comment.
There's a misleading warning log that emits during shutdown sometimes “Attempted to drop unknown FFI handle”, where this code can be re-entered (valid code path). This was just an easy fix to no-op / prevent that log in that case
e5fe5da to
19dd157
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Closed rooms retain WebRTC resources
inner.clone() creates a reference cycle for every connected room. The shared manager stores this callback in on_state_changed, while RoomSession owns that manager. Dropping a closed room retains its RTC engine and WebRTC factories indefinitely.
8b7f66c to
92e7203
Compare
3 participants
This PR addresses two issues related to memory lifecycle/safety:
CudaContextandFfiServercleanup on dispose.CudaContext:
CudaContextmultiple timesFfiServer: