Bugfix/133- fix util unit test memory leak #625
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- vnet::UdpConn holds Arc to vnet::ConnObserver, which would most of the time hold the Arc to vnet::UdpConn. We use Weak when pointing upward (i.e. parent)
- We replace the Arc pointing to parent with Weak
- util::UdpConn should not own the table of UdpConn because newly created util::UdpConn will always be added into the table. This table of UdpConn is sufficiently owned by Listener and the ListenConfig::read_loop.
- The Arc pointing to parent is replaced by Weak - Nic and RouterInternal should not point to each other. Make the nics table in RouterInternal Weak.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #625 +/- ##
==========================================
- Coverage 60.63% 60.62% -0.01%
==========================================
Files 471 471
Lines 48430 48429 -1
Branches 12280 12279 -1
==========================================
- Hits 29365 29361 -4
Misses 9676 9676
- Partials 9389 9392 +3 ☔ View full report in Codecov by Sentry. |
|
Could you fix clippy warning? |
|
clippy warning fixed. |
rainliu
approved these changes
Nov 3, 2024
tubzby
pushed a commit
to tubzby/webrtc-rs
that referenced
this pull request
Nov 26, 2024
* Remove cyclic dependency in vnet::UdpConn - vnet::UdpConn holds Arc to vnet::ConnObserver, which would most of the time hold the Arc to vnet::UdpConn. We use Weak when pointing upward (i.e. parent) * Remove cyclic dependency in vnet::resolver - We replace the Arc pointing to parent with Weak * Fix util::UdpConn owns it's own Arc - util::UdpConn should not own the table of UdpConn because newly created util::UdpConn will always be added into the table. This table of UdpConn is sufficiently owned by Listener and the ListenConfig::read_loop. * Remove cyclic dependency in vnet::router - The Arc pointing to parent is replaced by Weak - Nic and RouterInternal should not point to each other. Make the nics table in RouterInternal Weak. * Fix clippy warning --------- Co-authored-by: mutexd <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I use this crate in my project which is sort of a combination of
rtp-to-webrtcandrtp-forwarder. I came to realize that there's memory leak issue and I wish to tackle it. I bumped into Issue#133 and I guess I could start with it and see if some memory leaks can be resolved by tackling it.According to Issue#133. There is memory leak in Util/Turn/Ice/Interceptor detected by nightly-rusts sanitizer.
This PR targets the memory leak issue in Util and hopefully we can tackle other modules in the near future.
vnetis used in unit test and several cyclic dependencies are discovered and resolved by usingWeak<>util::UdpConnowns a map of UdpConn and itself is also added into the map, which creates a cyclic dependency ofArc<>and thus the memory will never be released. The way I see it, Listener already owns the table and should take care of clean-up. The only operationutil::UdpConndo to the map is clean-up the map uponclose. In my opinion it's superfluous and we should just remove the map fromutil::UdpConn.RUSTFLAGS="-Z sanitizer=leak" cargo testin util shows no sign of memory leak anymore.