Add packet timeout callback #50 (Rebase) #202
Conversation
Co-authored-by: Charlie Jonas <charlie@charliejonas.co.uk>
Co-authored-by: Charlie Jonas <charlie@charliejonas.co.uk>
Co-authored-by: Charlie Jonas <charlie@charliejonas.co.uk>
ping.go
Outdated
| @@ -503,6 +515,24 @@ func (p *Pinger) runLoop( | |||
| } | |||
| } | |||
|
|
|||
| func (p *Pinger) CheckInFlightPackets() { | |||
| // Loop through each item in map | |||
| if time.Duration(p.TTL) == maxPacketTimeout { | |||
There was a problem hiding this comment.
I think you misunderstand what TTL is. This is the ICMP packet network TTL. AKA, how many router hops. Not time.
There was a problem hiding this comment.
Not at all; it would be more accurate to say I misunderstand the codebase and changes, and admittedly did not take much time to understand it. 🙃 I saw TTL added as a new property since @hamptonmoore's original PR, which had property PacketTimeout, and made a fast correlation between the two. I will go back through @hamptonmoore's original PR and re-implement the PacketTimeout property as it was before.
ping.go
Outdated
| ipv4: false, | ||
| network: "ip", | ||
| protocol: "udp", | ||
| InFlightPackets: firstSequence, |
There was a problem hiding this comment.
This is intentionally not exported, it's only for internal use to avoid data races.
There was a problem hiding this comment.
Understood. However, it was originally exported in @hamptonmoore's PR, so I left it as-is. I will adjust it as private in my next commit.
ping.go
Outdated
| return | ||
| } | ||
| currentTime := time.Now() | ||
| for id, inflight := range p.InFlightPackets { |
There was a problem hiding this comment.
This seems like a very expensive way to deal with this. I need to think about if there are better solutions than walking the whole map every time.
There was a problem hiding this comment.
I don't disagree, however I just adapted the original implementation from #176, which seemed to be accepted at the time. While you're considering a better solution, I'll do the same as I go through the codebase for the other code reviews and see if anything stands out.
There was a problem hiding this comment.
My original though on this was to spawn a time.Timer() that would send the timeout down the main packet watching channel. If a packet arrives before the timer ends, we .Stop() it.
What do you think?
|
See #226 |
Hello,
I've been following #176 for a little while, and figured I'd take a stab at rebasing the work done by @hamptonmoore. I'm hoping I did it correctly; if not, feel free to request changes as needed and I'll do my best. I'd love to see this get merged in.
Thanks!