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

Implement a per-packet timeout (i.e. -W in UNIX ping) #63

Closed
Lvzhenqian opened this issue Jun 6, 2019 · 11 comments
Closed

Implement a per-packet timeout (i.e. -W in UNIX ping) #63

Lvzhenqian opened this issue Jun 6, 2019 · 11 comments

Comments

@Lvzhenqian
Copy link

Lvzhenqian commented Jun 6, 2019

335          fmt.Println("get package: ",p.PacketsRecv)
336		if p.Count > 0 && p.PacketsRecv >= p.Count {
			close(p.done)
			wg.Wait()
			return
		}

change p.PacketsRecv to p.PacketsSent it work , but it impossible to collect the last package after using p.PacketsSent :(

24 bytes from 52.34.112.193: icmp_seq=0 time=186.8937ms
get package:  1
get package:  1
get package:  1
24 bytes from 52.34.112.193: icmp_seq=2 time=188.8935ms
get package:  2
get package:  2
get package:  2
24 bytes from 52.34.112.193: icmp_seq=4 time=189.8887ms
get package:  3
get package:  3
24 bytes from 52.34.112.193: icmp_seq=5 time=188.89ms
get package:  4
get package:  4
24 bytes from 52.34.112.193: icmp_seq=6 time=188.8929ms
get package:  5
get package:  5
24 bytes from 52.34.112.193: icmp_seq=7 time=188.8929ms
get package:  6
get package:  6
24 bytes from 52.34.112.193: icmp_seq=8 time=195.8847ms
get package:  7
get package:  7
get package:  7
get package:  7
get package:  7
get package:  7
get package:  7
get package:  7
get package:  7
@sparrc
Copy link
Member

sparrc commented Jun 6, 2019

Sorry I'm not sure I understand, can you explain what you are trying to accomplish?

@Lvzhenqian
Copy link
Author

func worker(addr string, count int) *ping.Statistics {
	pinger, CreateErr := ping.NewPinger(addr)
	if CreateErr != nil {
		panic(CreateErr)
	}
	pinger.SetPrivileged(true)
	pinger.Count = count
	pinger.OnRecv = func(pkt *ping.Packet) {
		fmt.Printf("%d bytes from %s: icmp_seq=%d time=%v\n",
			pkt.Nbytes, pkt.IPAddr, pkt.Seq, pkt.Rtt)
	}
	pinger.Run()
	return pinger.Statistics()
}

func main() {
	s := worker("52.34.112.193",10)
	fmt.Println(s)
}

This is my code, and when I tested it, I found that when the network was not stable enough to recv a packet or two, the count never met and quit the ping program

@Lvzhenqian
Copy link
Author

Sorry I'm not sure I understand, can you explain what you are trying to accomplish?

It should exit the ping program after sending 10 packets

@sparrc
Copy link
Member

sparrc commented Jun 7, 2019

You can use the Timeout parameter on the Pinger struct, does that work for you?

see https://godoc.org/github.com/sparrc/go-ping#Pinger

@Lvzhenqian
Copy link
Author

Lvzhenqian commented Jun 14, 2019

case <-interval.C:
if p.Count > 0 && p.PacketsSent >= p.Count {
       // before is continue , that is why can't exit pinger
	close(p.done)
	wg.Wait()
}

Timeout this parameter does not meet my requirements...en

@sparrc
Copy link
Member

sparrc commented Jun 14, 2019

Fundamentally the pinger "count" option waits for responses after it sends out pings so it can't simply exit immediately after sending 10 pings. This is consistent with the BSD implementation of ping but I suppose it's not consistent with the GNU implementation.

One option would be to make it consistent with the GNU implementation, although I think the BSD implementation seems more reasonable.

Another option would be to implement a per-ping timeout (-W in GNU and BSD ping) in addition to the current overall timeout, though I'm not sure this exactly fills your need because you would need to set the timeout to something where pings could actually be received.

Lastly there could simply be a boolean switch to have the Count parameter act like GNU ping and exit after sending the last ping.

@Lvzhenqian
Copy link
Author

My requirement is to send a packet, wait for the duration of receiving a packet, increase the timeout count if failure and exit the program after waiting for receiving 10 packets, regardless of whether the 10 packets have timeouts or not.

I think the Boolean switch is a great option for exiting ping.

@sparrc
Copy link
Member

sparrc commented Jun 18, 2019

wait for the duration of receiving a packet

Do you mean you want to wait for Interval seconds, or you want another configurable option for this duration?

@Lvzhenqian
Copy link
Author

Yes, wait for interval seconds. When timeout should be interval+timeout to send again

@sparrc sparrc changed the title when lost a package can't stop pinger from counts... Implement a per-ping timeout (ie, -W in unix ping) Jun 25, 2019
@CHTJonas CHTJonas changed the title Implement a per-ping timeout (ie, -W in unix ping) Implement a per-packet timeout (i.e. -W in UNIX ping) Aug 25, 2021
@cyounkins
Copy link

First, thank you for this project!

Without per-packet timeout, the problem I see is that when sending a set of probes, the earlier probes have more time to have responses. Say you send two probes, one at each t=0 and t=1 seconds. Then either deadline or control-C stops listening for replies at t=2. If the response time was a constant 1500ms, the first probe would receive a reply but the second would count as a loss. Max response time is equally problematic and tends to increase with larger count.

It's incorrect to interpret the packet loss results of any ping tool as "the client never responded" since the tools can't show that the remote won't reply, say, tomorrow. Of course a delay of hours is unlikely but when dealing with troubled, congested and highly buffered networks, tens of seconds is not unheard of.

Tools and libraries should permit a clear interpretation of packet loss. If you introduce a per-probe timeout, ignore replies past the timeout and wait the duration after sending the last probe, then packet loss can be interpreted as "probes that did not receive a reply within X time". All response times are then bounded.

Side note: What is called 'timeout' in this project is 'deadline' in UNIX ping, and I think the UNIX naming makes sense when both are used. 'Deadline' is when everything has to be done.

Is this something the developers would be interested in seeing added?

@stanimirivanovde
Copy link

Looks like I opened a dup: #209

@SuperQ SuperQ closed this as completed Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants