fix(mem-leak): running_swap never shrinks
#2301
Conversation
|
Why is this PR blocked? I can see CI builds are green and commit history doesn't depend on any common commits from other PRs. |
Nope it depends on history from #2280, no extra commits are showing up here since I am trying to merge into |
mm2src/mm2_main/src/lp_swap.rs
Outdated
| pub fn running_swaps_num(ctx: &MmArc) -> u64 { | ||
| let swap_ctx = SwapsContext::from_ctx(ctx).unwrap(); | ||
| let swaps = swap_ctx.running_swaps.lock().unwrap(); | ||
| swaps.iter().fold(0, |total, (swap, _)| match swap.upgrade() { | ||
| Some(_) => total + 1, | ||
| None => total, | ||
| }) | ||
| let count = swap_ctx.running_swaps.lock().unwrap().len(); | ||
| count as u64 | ||
| } |
There was a problem hiding this comment.
let change the return type to usize instead of u64
mm2src/mm2_main/src/lp_swap.rs
Outdated
| for (swap, _) in swaps.values() { | ||
| if coins.contains(&swap.maker_coin().to_string()) || coins.contains(&swap.taker_coin().to_string()) { | ||
| uuids.push(*swap.uuid()) |
There was a problem hiding this comment.
you can use uuid.extend and filter_map to avoid for loop and push
I am not sure what is the purpose of this PR then 🤔 |
|
Basically this PR looks good for me: it removes finished or aborted swaps from the running_swaps list. |
this field was convereted to a hashmap instead of a vector for easy access to the swap also we now manually delete the swap from running swaps when the swap is finished (memory leak fix). as a consequence to manuallly deleting the swap from running_swaps, we can now store them as arcs instead of weakrefs, which simplifies a lot of .upgrade calls.
mariocynicys
force-pushed
the
running-swaps-memleak
branch
from
b02fced to
0890de8
Compare
|
re-wrote the fix on top of dev. it's not blocked anymore by the original PR from |
| running_swaps: Mutex<Vec<Weak<dyn AtomicSwap>>>, | ||
| running_swaps: Mutex<HashMap<Uuid, Arc<dyn AtomicSwap>>>, |
There was a problem hiding this comment.
I wonder if we intentionally chose a weak reference instead of Arc as it might avoid cyclic references (similar to how it's done for MmArc with MmWeak in coins) 🤔
There was a problem hiding this comment.
I suspect this could be the reason of https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/12666194163/job/35297118282?pr=2301#step:9:895 failure.
There was a problem hiding this comment.
not that i can trivially find the AtomicSwap pointing back to it self (points to mmctx tho:
you see, such questions are really hard to answer in such a codebase and require digging xD
it's much easier/managable to avoid the arc/weak model and use our abortable systems to make sure cyclic refs aren't problematic.
i went for arc here tho since we already manually remove the swap after it finishes or after stopping it. since we drop the thing manually we don't need to complicate it with weak refs.
There was a problem hiding this comment.
It's worth to test that change (by using test_mm2_stops_immediately test) to make sure that's not the case.
onur-ozkan
force-pushed
the
running-swaps-memleak
branch
from
a4e7c75 to
fbd5e9f
Compare
|
Force pushed to re-trigger CI. |
THERE ARE NO RUNNING SWAPS
theoretically we don't need to clear it on native targets since the executable will terminate even without clearing, but for wasm the story isn't exactly the same, i presume
this field was convereted to a hashmap instead of a vector for easy access to the swap. also we now manually delete the swap from running swaps when the swap is finished~/inturepted~ (memory leak fix). as a consequence to manuallly deleting the swap from running_swaps, we can now store them as arcs instead of weakrefs, which simplifies a lot of
.upgrade()calls.blocked on its base #2280re-fixed on top ofdevinstead