fix(mem-leak): `running_swap` never shrinks by mariocynicys · Pull Request #2301 · GLEECBTC/komodo-defi-framework

mariocynicys · 2024-12-23T11:40:19Z

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 #2280~~ re-fixed on top of dev instead

onur-ozkan · 2024-12-23T12:09:48Z

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.

mariocynicys · 2024-12-23T12:20:56Z

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 robust-swaps not dev.

borngraced

couple minor notes

borngraced · 2024-12-23T12:10:22Z

mm2src/mm2_main/src/lp_swap.rs

 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
 }


let change the return type to usize instead of u64

borngraced · 2024-12-23T12:59:49Z

mm2src/mm2_main/src/lp_swap.rs

+    for (swap, _) in swaps.values() {
+        if coins.contains(&swap.maker_coin().to_string()) || coins.contains(&swap.taker_coin().to_string()) {
+            uuids.push(*swap.uuid())


you can use uuid.extend and filter_map to avoid for loop and push

onur-ozkan · 2024-12-23T16:21:23Z

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 robust-swaps not dev.

I am not sure what is the purpose of this PR then 🤔

dimxy · 2024-12-26T05:39:43Z

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 · 2024-12-30T08:57:30Z

re-wrote the fix on top of dev. it's not blocked anymore by the original PR from robust-swaps branch.
there is no notion of aborability/stopping of running swaps (only finishing), so that will be covered in the original PR from robust-swaps.

onur-ozkan · 2025-01-08T07:43:30Z

mm2src/mm2_main/src/lp_swap.rs


 struct SwapsContext {
-    running_swaps: Mutex<Vec<Weak<dyn AtomicSwap>>>,
+    running_swaps: Mutex<HashMap<Uuid, Arc<dyn AtomicSwap>>>,


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) 🤔

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.

not that i can trivially find the AtomicSwap pointing back to it self (points to mmctx tho: https://github.com/KomodoPlatform/komodo-defi-framework/blob/0a9410258c7bf58e78d26b33b7a0ffb749c7ba16/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L213)
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.

It's worth to test that change (by using test_mm2_stops_immediately test) to make sure that's not the case.

oops

onur-ozkan · 2025-01-08T07:44:47Z

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

mariocynicys · 2025-01-16T09:56:08Z

mm2src/mm2_main/src/lp_swap.rs

@@ -655,13 +655,9 @@ pub fn get_locked_amount(ctx: &MmArc, coin: &str) -> MmNumber {
 }

 /// Get number of currently running swaps


fix this doc comment
also consider clearing the running swap on mmctx destructor/stop method instead.

also i wanted to clear the swaps in MmCtx::stop instead since that's how we destruct MmCtx. but that won't work because we have no access to SwapsContext data type.

onur-ozkan · 2025-01-21T07:37:46Z

mm2src/mm2_main/src/lp_swap.rs

+pub fn clear_running_swaps(ctx: &MmArc) {
    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,
-    })
+    swap_ctx.running_swaps.lock().unwrap().clear();


Does clearing the map stops swap processes?

~~yup. the store of running_swaps stores the swaps (AtomicSwap object) along with the abort handles (which will trigger on Drop) they are running on.~~
messed things, look at: #2301 (comment)

Can you please mention that on the code?

https://github.com/KomodoPlatform/komodo-defi-framework/blob/dd5418fc2444679a57acdb5370788b2ac1fcc1da/mm2src/mm2_main/src/lp_swap.rs#L657-L660

it's already in the doc comment. or u mean somewhere else?

Ah didn't see that. I was referring to caller side but it's fine

big ooops:

yup. the store of running_swaps stores the swaps (AtomicSwap object) along with the abort handles (which will trigger on Drop) they are running on.

this is completely wrong. this feat is in another PR. i mixed things up.

no, clearing the running swaps will not stop these swaps. (though could be stopped via other means, e.g. abort_all from a bigger subsystem).

Ok that is def. worth to mention that on the line.

I tried this code, looks like clear_running_swaps fn is needed to clear swaps_ctx in MmCtx to allow to release ctx so stop_and_wait_for_ctx_is_dropped fn can finish (used in wasm tests).

(this note to memorise)

updated the doc comment accordingly till the other PR that stores the abort handle lands in.

dimxy · 2025-01-21T17:26:57Z

I am okay with this PR.
I see @borngraced has changes suggested, and we could merge it after this is resolved.

borngraced · 2025-01-22T13:36:07Z

I am okay with this PR. I see @borngraced has changes suggested, and we could merge it after this is resolved.

Seems the changes to the code were reverted. So I approved

* dev: fix(hash-types): remove panic, enforce fixed-size arrays (GLEECBTC#2279) fix(ARRR): store unconfirmed change output (GLEECBTC#2276) feat(tendermint): staking/delegation (GLEECBTC#2322) chore(deps): `timed-map` migration (GLEECBTC#2247) fix(mem-leak): `running_swap` never shrinks (GLEECBTC#2301) chore(dep-bump): libp2p (GLEECBTC#2326) refactor(build script): rewrite the main build script (GLEECBTC#2319)

* dev: fix(derive_key_from_path): check length of current_key_material (#2356) chore(release): bump mm2 version to 2.4.0-beta (#2346) fix(tests): add additional testnet sepolia nodes to test code (#2358) fix(swaps): maintain legacy compatibility for negotiation messages (#2353) refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354) feat(tpu-v2): provide swap protocol versioning (#2324) feat(wallet): add change mnemonic password rpc (#2317) fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (#2261) feat(tendermint): unstaking/undelegation (#2330) fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (#2333) feat(event-streaming): API-driven subscription management (#2172) fix(hash-types): remove panic, enforce fixed-size arrays (#2279) fix(ARRR): store unconfirmed change output (#2276) feat(tendermint): staking/delegation (#2322) chore(deps): `timed-map` migration (#2247) fix(mem-leak): `running_swap` never shrinks (#2301) chore(dep-bump): libp2p (#2326) refactor(build script): rewrite the main build script (#2319)

* dev: (24 commits) fix(eth-tpu): remove state from funding validation (GLEECBTC#2334) improvement(rpc-server): rpc server dynamic port allocation (GLEECBTC#2342) fix(tests): fix or ignore unstable tests (GLEECBTC#2365) fix(fs): make `filter_files_by_extension` return only files (GLEECBTC#2364) fix(derive_key_from_path): check length of current_key_material (GLEECBTC#2356) chore(release): bump mm2 version to 2.4.0-beta (GLEECBTC#2346) fix(tests): add additional testnet sepolia nodes to test code (GLEECBTC#2358) fix(swaps): maintain legacy compatibility for negotiation messages (GLEECBTC#2353) refactor(SwapOps): impl defaults for protocol specific swapops fns (GLEECBTC#2354) feat(tpu-v2): provide swap protocol versioning (GLEECBTC#2324) feat(wallet): add change mnemonic password rpc (GLEECBTC#2317) fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (GLEECBTC#2261) feat(tendermint): unstaking/undelegation (GLEECBTC#2330) fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (GLEECBTC#2333) feat(event-streaming): API-driven subscription management (GLEECBTC#2172) fix(hash-types): remove panic, enforce fixed-size arrays (GLEECBTC#2279) fix(ARRR): store unconfirmed change output (GLEECBTC#2276) feat(tendermint): staking/delegation (GLEECBTC#2322) chore(deps): `timed-map` migration (GLEECBTC#2247) fix(mem-leak): `running_swap` never shrinks (GLEECBTC#2301) ...

mariocynicys added the status: blocked label Dec 23, 2024

borngraced suggested changes Dec 23, 2024

View reviewed changes

mariocynicys changed the base branch from robust-swaps to dev December 29, 2024 13:41

mariocynicys force-pushed the running-swaps-memleak branch from b02fced to 0890de8 Compare December 30, 2024 08:55

mariocynicys added status: pending review and removed status: blocked labels Dec 30, 2024

onur-ozkan reviewed Jan 8, 2025

View reviewed changes

remove irrelevant comment

fbd5e9f

oops

onur-ozkan force-pushed the running-swaps-memleak branch from a4e7c75 to fbd5e9f Compare January 8, 2025 07:44

mariocynicys added 2 commits January 8, 2025 12:42

test fix: don't wait for running swaps

f5ac03d

THERE ARE NO RUNNING SWAPS

test fix: proactively clear the running swaps map

6f157d6

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

laruh added the priority: high Important tasks that need attention soon. label Jan 16, 2025

mariocynicys commented Jan 16, 2025

View reviewed changes

fix clear running_swaps doc comment

dd5418f

mariocynicys requested review from borngraced and onur-ozkan January 20, 2025 11:10

onur-ozkan reviewed Jan 21, 2025

View reviewed changes

onur-ozkan previously approved these changes Jan 21, 2025

View reviewed changes

borngraced previously approved these changes Jan 21, 2025

View reviewed changes

clear up the doc comment confusion

64a0012

mariocynicys dismissed stale reviews from borngraced and onur-ozkan via 64a0012 January 22, 2025 10:46

shamardy approved these changes Jan 22, 2025

View reviewed changes

shamardy merged commit da29651 into dev Jan 22, 2025
19 of 23 checks passed

shamardy deleted the running-swaps-memleak branch January 22, 2025 17:31

shamardy pushed a commit that referenced this pull request Jan 28, 2025

fix(mem-leak): running_swap never shrinks (#2301)

26f1c62

shamardy pushed a commit that referenced this pull request Jan 30, 2025

fix(mem-leak): running_swap never shrinks (#2301)

ef8c254

shamardy pushed a commit that referenced this pull request Jan 30, 2025

fix(mem-leak): running_swap never shrinks (#2301)

9e65e7a

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

		@@ -655,13 +655,9 @@ pub fn get_locked_amount(ctx: &MmArc, coin: &str) -> MmNumber {
		}

		/// Get number of currently running swaps

Conversation

mariocynicys commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onur-ozkan commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariocynicys commented Dec 23, 2024

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented Dec 23, 2024

Uh oh!

dimxy commented Dec 26, 2024

Uh oh!

mariocynicys commented Dec 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented Jan 8, 2025

Uh oh!

mariocynicys Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocynicys Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimxy commented Jan 21, 2025

Uh oh!

borngraced commented Jan 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mariocynicys commented Dec 23, 2024 •

edited

Loading

onur-ozkan commented Dec 23, 2024 •

edited

Loading

mariocynicys Jan 16, 2025 •

edited

Loading

mariocynicys Jan 21, 2025 •

edited

Loading