Reorder channel selection order and make Stop() idempotent

go-ping / ping

ICMP Ping library for Go

MIT License

1.32k stars 344 forks source link

Reorder channel selection order and make Stop() idempotent #159

Closed CHTJonas closed 3 years ago

CHTJonas commented 3 years ago

This PR:

Reorders the channels in the main select clause of the Run() function so that we always deal with received packets first rather than sending new packet out. This is so that the TTL values don't go stale when pinging very rapidly (cf. #154).
Improves CPU usage slightly by greedily calling Stop() on tickers where possible.
Removes the initial packet sent around line 403 which I can't really see a need for.

SuperQ commented 3 years ago

I think I prefer to stick to the defer Stop() method. Having to maintain stops on all function exits is fragile.

CHTJonas commented 3 years ago

How about a single function which makes all the necessary Stop() calls? I could factor this out into a separate PR if necessary (and maybe merge with parts of #116 minus the error bit)?

SuperQ commented 3 years ago

It's not the number of function calls, it's the fact that you need to make sure they're all there at any exit from the function. This is why defer is used.

CHTJonas commented 3 years ago

Updated to use the defer-style syntax which I think looks better. It's objectively less code to worry about/forget in that select clause. I've also included the idempotent pinger.Stop() because it made sense to.

CHTJonas commented 3 years ago

This also fixes #157.

CHTJonas commented 3 years ago

Yep correct on all accounts @sparrc!

chenk008 commented 3 years ago

In my opinion, there is no guarantee order in select clause.