RunLoop issue, after Stop() is issued

[asmpro]

Hi, I noticed, that Stop() function does not write bool to done channel, but instead reads from it.
In my code I am using the following:

p.RunLoop()
<-p.Done()
if err = p.Err(); err != nil {
	return result, err
}

which deadlocks, so I suggest Stop() function should be changed to:

func (p *Pinger) Stop() {
p.debugln("Stop(): close(p.ctx.stop)")
close(p.ctx.stop)
p.debugln("Stop(): p.ctx.done <- true")
p.ctx.done <- true
}

Regards,
Uros

[asmpro]

Forgot to write, that in my code OnIdle function calls p.Stop() after N idle calls.

Regards,
Uros

[bewiwi]

I use RunLoop in the same way and p.Stop() don't stop the loop.

Here a little example:

	p := fastping.NewPinger()

	ra, err := net.ResolveIPAddr("ip4:icmp", "127.0.0.1")
	if err != nil {
		fmt.Println(err)
	}
	p.AddIPAddr(ra)
	p.OnRecv = func(addr *net.IPAddr, rtt time.Duration) {
		fmt.Println("receive")
		p.Stop()
	}
	p.OnIdle = func() {
		fmt.Println("idle")
		p.Stop()
	}
	p.RunLoop()
	ticker := time.NewTicker(time.Second * 2)
	select {
	case <-p.Done():
		fmt.Println("loop done")
	case <-ticker.C:
		fmt.Println("timeout done")
		break
	}
	ticker.Stop()

Stdout result is :

receive
timeout done

I just ping loopback with 2 sec of "timeout" and "loop done" was never printed.

[cova-fe]

I'm facing the same issue, but I see in mainloop this check:
mainloop:
for {
select {
case <-p.ctx.stop:
p.debugln("Run(): <-p.ctx.stop")

So shouldn't

func (p *Pinger) Stop()

return something like

<-p.ctx.stop

instead of
<- p.ctx.done

?

[asmpro]

cova-fe:
I am almost sure the little bug is fixed by changing line 377 in Stop() function of fastping.go from:

<-p.ctx.done

to:

p.ctx.done <- true

You see, the problem in my opinion is in the fact, that Done() function in fact returns done channel, which must contain a boolean true value if you want to stop the loop.

I think that your loop should look similar to bewiwi's, so checking the p.Done() and not the p.ctx.stop itself.

This patch is tested and the code is running in production.

Regards,
Uros

[suvvari8]

@asmpro Hi, I have a func here which has to run forever runloop and break whenever there is ping failure. The problem is it works fine till it can ping and goes to idle but doesn't stop or exit. Here is the code any help is appreciated. Thanks

func pingIP(ipadd string) {
pinger := fastping.NewPinger()
err := pinger.AddIP(ipadd)
if err != nil {
fmt.Printf("Error adding IP Address")
panic(err)
}
pinger.OnRecv = func(addr *net.IPAddr, rtt time.Duration) {
fmt.Printf("IP Addr: %s receive, RTT: %v\n", addr.String(), rtt)
}
// pinger.MaxRTT = time.Second
// ticker := time.NewTicker(time.Second * 10)
pinger.RunLoop()
select {
case <-pinger.Done():
if err := pinger.Err(); err != nil {
fmt.Printf("Ping failed: %v", err)
}
// case <-ticker.C:
// break
}
// ticker.Stop()
pinger.Stop()
os.Exit(1)
}

[asmpro]

@suvvari8 Hi,

First patch fastping.go as I mentioned in my first comment.

Then use RunLoop this way:

type PingData struct {
PingReplies uint32
LastPingRTT time.Duration
}
type PingResult map[string]*PingData

p := fastping.NewPinger()
pingRes := PingResult{}
// Add ips
p.AddIPAddr(ip)
pingRes[ip] = &PingData{}
// ... more ips (do not forget to set pingRes for each IP)
// Set nping to number of pings per IP, you wish to process, for example 3
npings := 3
p.OnRecv = func(addr *net.IPAddr, rtt time.Duration) {
addrStr := addr.String()
ips[addrStr].PingReplies++
ips[addrStr].LastPingRTT = rtt
}

pings := 0
p.OnIdle = func() {
pings++
if pings >= npings {
p.Stop()
}
}

p.RunLoop()
<-p.Done()
if err = p.Err(); err != nil {
panic(err)
}

[ghostiam]

I seem to have found the cause of deadlock.
https://github.com/tatsushid/go-fastping/blob/master/fastping.go#L454

Since the code is executed in parallel, the operation time.NewTicker(p.MaxRTT) conflicts with "close (p.ctx.stop)" and when processing anything in

OnIdle = func () {
} 

we get deadlock

Solution: do not use blocking operations in OnIdle

pings := 0
p.OnIdle = func() {
    pings++
    if pings >= npings {
-      p.Stop()
+      go p.Stop()
    }
}

or not use OnIdle at all.