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

Harden and rationalize c-ares timeout computation #187

Merged
merged 2 commits into from May 6, 2018

Conversation

Projects
None yet
3 participants
@b-spencer
Contributor

b-spencer commented Apr 23, 2018

(This should be a clean version of PR #186. Sorry about the noise.)

When c-ares sends a DNS query, it computes the timeout for that request as follows:

timeplus = channel->timeout << (query->try_count / channel->nservers);
timeplus = (timeplus * (9 + (rand () & 7))) / 16;

I see two issues with this code. Firstly, when either try_count or channel->timeout are large enough, this can end up as an illegal shift.

Secondly, the algorithm for adding the random timeout (added in 2009) is surprising. The original commit that introduced this algorithm says it was done to avoid a "packet storm". But, the algorithm appears to only reduce the timeout by an amount proportional to the scaled timeout's magnitude. It isn't clear to me that, for example, cutting a 30 second timeout almost in half to roughly 17 seconds is appropriate. Even with the default timeout of 5000 ms, this algorithm computes values between 2812 ms and 5000 ms, which is enough to cause a slightly latent DNS response to get spuriously dropped.

If preventing the timers from all expiring at the same time really is desirable, then it seems better to extend the timeout by a small factor so that the application gets at least the timeout it asked for, and maybe a little more. In my experience, this is common practice for timeouts: applications expect that a timeout will happen at or after the designated time (but not before), allowing for delay in detecting and reporting the timeout. Furthermore, it seems like the timeout shouldn't be extended by very much (we don't want a 30 second timeout changing into a 45 second timeout, either).

Consider also the documentation of channel->timeout in ares_init_options():

The number of milliseconds each name server is given to respond to a query on the first try. (After the first try, the timeout algorithm becomes more complicated, but scales linearly with the value of timeout.) The default is five seconds.

In the current implementation, even the first try does not use the value that the user supplies; it will use anywhere between 56% and 100% of that value.

The attached patch attempts to address all of these concerns without trying to make the algorithm much more sophisticated. After performing a safe shift, this patch simply adds a small random timeout to the computed value of between 0 ms and 511 ms. I could see limiting the random amount to be no greater than a proportion of the configured magnitude, but I can't see scaling the random with the overall computed timeout. As far as I understand, the goal is just to schedule retries "not at the same exact time", so a small difference seems sufficient.

It might be useful to add an option that disables the timeout randomization. I'm not even sure I understand what problems it addresses, exactly. UDP packet loss?

What do you think?

@bradh352

This comment has been minimized.

Member

bradh352 commented Apr 23, 2018

Your logic seems good to me, I wasn't even aware the rand() was occurring.

+1 for making the rand() configurable.

@bagder

This comment has been minimized.

Member

bagder commented Apr 23, 2018

Sounds good to me. I think I could even consider dropping the random part completely and push that responsibility to the user if its really wanted. I can't recall the discussion from when this was added.

@b-spencer

This comment has been minimized.

Contributor

b-spencer commented Apr 23, 2018

I was surprised to see the rand(), too. I can't think of a reason that any of my use cases would want it there, really.

I agree that a slightly randomized user-configured initial timeout would seem to also lead to the effect of a burst of requests all not quite timing out at the same time, which is probably good enough.

And it's certainly easier to remove the rand() step than add a configuration parameter. I'll wait a bit for any other comments, and then submit a version with the rand() removed if nobody objects.

@bradh352 bradh352 self-requested a review Apr 29, 2018

@bradh352

looks good to me

@bradh352 bradh352 requested a review from bagder May 5, 2018

@bagder

bagder approved these changes May 5, 2018

👍

@bradh352 bradh352 merged commit 1a4ccdc into c-ares:master May 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment