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

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

Conversation

danielealbano
Copy link
Contributor

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.

@danielealbano danielealbano changed the title Ensure that the context cancel and deadline are respected Ensure that the context cancel and deadline are respected and the related errors properly propagated. Apr 28, 2024
@danielealbano danielealbano changed the title Ensure that the context cancel and deadline are respected and the related errors properly propagated. Ensure that the context cancel and deadline are respected and the related errors properly propagated Apr 28, 2024
@danielealbano danielealbano changed the title Ensure that the context cancel and deadline are respected and the related errors properly propagated Ensure that the context cancel and deadline hit are respected and the related errors properly propagated Apr 28, 2024
@r3inbowari
Copy link
Collaborator

r3inbowari commented Apr 28, 2024

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
Copy link
Contributor Author

danielealbano commented Apr 28, 2024

@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
Copy link
Collaborator

r3inbowari commented Apr 28, 2024

@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.

@r3inbowari r3inbowari merged commit f5ed869 into showwin:master Apr 28, 2024
@r3inbowari r3inbowari self-requested a review April 28, 2024 21:35
@danielealbano danielealbano deleted the ensure-context-cancel-and-deadline-are-respected branch April 28, 2024 22:00
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

Successfully merging this pull request may close these issues.

2 participants