search

showwin / speedtest-go

CLI and Go API to Test Internet Speed using speedtest.net
MIT License
565 stars 115 forks source link

Ensure that the context cancel and deadline hit are respected and the related errors properly propagated #188

Closed danielealbano closed 7 months ago

danielealbano commented 7 months ago

This PR improves the behaviour of how the context is handled in HTTPPing.

Currently, if a context is cancelled, the HTTPPing will keep trying executing the requests, failing them, to return at the end a timeout error, therefore misreporting the reason for which the requests are actually failing. On top of this, if a number of pings passed before the context got cancelled or the deadline was hit, the error will not be reported at all, making it hard for the caller to understand if the operation was succesful or not.

In my specific case I need to ensure that if I don't get a response within 30/60 seconds the speedtest terminates and reports the reason for the termination and in case it's a timeout it doesn't report general errors.

The PR ensure that if a context cancelled or a deadline hit, the code ping terminates and returns whatever latencies it collected, also returning the appropriate error.

This PR is only for HTTPPing but TCPPing has the same issue, not sure if it applies to ICMPPing but it might be the same there.

r3inbowari commented 7 months ago

In addition, could you compare the lentency difference between tcp ping and http ping? I found that they seem to be twice as different(http ping seems to be half as much.).

image

danielealbano commented 7 months ago

@r3inbowari I did noticed that anomaly as well but the reason seems to be very simple, there is a missing / 2 :)

Although from looking at it, I think that the HTTPPing is wrong, there shouldn't be / 2, infact it's not in the ICMPPing neither.

The ping usually measure the RTT (roundtrip time) which is the forth and back, not just the forth. The same in the docs from speedtest https://help.speedtest.net/hc/en-us/articles/360038679354-How-does-Speedtest-measure-my-network-speeds.

Before fixing it though I will wait for a feedback though.

r3inbowari commented 7 months ago

@danielealbano let's merge this first, thank you!

image

HTTPPing is wrong, there shouldn't be / 2

The first request is 2rtt. Yes, it should be fixed.