Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

mariocynicys
Copy link
Collaborator

@mariocynicys mariocynicys commented Dec 23, 2024

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
Copy link
Member

onur-ozkan commented Dec 23, 2024

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
Copy link
Collaborator Author

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.

Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple minor notes

Comment on lines 672 to 662
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
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let change the return type to usize instead of u64

Comment on lines 704 to 692
for (swap, _) in swaps.values() {
if coins.contains(&swap.maker_coin().to_string()) || coins.contains(&swap.taker_coin().to_string()) {
uuids.push(*swap.uuid())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@onur-ozkan
Copy link
Member

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
Copy link
Collaborator

dimxy commented Dec 26, 2024

Basically this PR looks good for me: it removes finished or aborted swaps from the running_swaps list.

@mariocynicys mariocynicys changed the base branch from robust-swaps to dev December 29, 2024 13:41
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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants