search

bparli / fastping-rs

ICMP ping library in Rust inspired by go-fastping and AnyEvent::FastPing Perl module
MIT License
75 stars 12 forks source link

Creating multiple Pinger instances on a recurring task/thread results in a threads "leak". #25

Open iMalinois opened 3 years ago

iMalinois commented 3 years ago

I have an app that occasionally spawns a new Pinger instance in a new task to do a few pings and I've noticed that this app keeps on facing a memory issue that originated from the code that creates the Pinger instances, resulting in my app being unable to spawn new threads. When I've further investigated it, I found out what I believe to be the root cause of the problem - the Pinger's listeners.

When a new Pinger instance is created using the Pinger::new method, two new threads are created, an IPv4 Listener and an IPv6 Listener. from looking at their code, those threads are infinite, and the only. time they return is if there is an error with the std::sync::mpsc::channel and the Pinger is marked as stopped (Which applies for both a continuous pinger and a ping_once pinger).

If I understood everything correctly and didn't miss anything (The threads in my app kept increasing by 2 each time which seems like pointing directly toward those listeners), I believe that there should be a way to stop those listeners - either with stop_pinger actually stopping them or having a pinger_shutdown method that make those threads stop (And probably pair that method with a start/restart method to restart those listeners).

Thank you!

bparli commented 3 years ago

hey @iMalinois, yes, stop is meant to merely stop pinging, not shutdown. Myabe I should have called it pause instead. The crate is written with the intention of the pingers being dynamic and useful (add/remove ping targets) as long as the program is running. If the pinger goes out of scope somehow it will essentially be shutdown as you noticed.

I'm curious why you create new instances of the pinger since it sounds like all those pingers are within the same scope?

iMalinois commented 3 years ago

Hey @bparli, thank you for the response. As for my scenario, my program periodically spawns different jobs, with some of those jobs having to ping different targets. those are async tasks that I spawn with tokio. Now, I don't wish to always ping those targets but rather ping them only when there is a job that needs to do that, and in those jobs I only wish to know if the ping was successful for the single target I'm communicating with. Therefore, instead of creating a single pinger instance and sharing it across the different threads and tasks, I assumed that I can just create a pinger instance on demand, as I didn't take in account the overhead of spawning such instance.

bparli commented 3 years ago

I see @iMalinois. I'm glad you're trying to use this crate, but tbh I'm not sure it aligns with your use case very well since this is meant to be a single long running instance with many ping targets.

Have you come across tokio-ping? At a glance, it appears to better align with what you describe (at least how I'm interpreting it)

iMalinois commented 3 years ago

Hey @bparli, I'll give it a try, thank you for your help and keep with the great work on that crate!