vshymanskyy / aiodns

A small async DNS client for MicroPython
MIT License
9 stars 1 forks source link

A query about timing #4

Closed peterhinch closed 5 days ago

peterhinch commented 5 days ago

The time calculations here don't look quite right. The value dt is a time difference (delta) and is based on modular arithmetic. At some point it wraps round from a high value to a low one.

The ticks_add method (L158) normally takes an "absolute" ticks value and adds a delta, producing a target "absolute" ticks value. I'm not sure that calling it with two delta values produces a sensible result, calling into question the < operator in L147.

I'm not 100% sure of my facts here, but in my code I adopt the following rules:

  1. Maintain a clear distinction between "absolute" ticks values and deltas.
  2. Only use comparison operators on deltas e.g. if ticks_diff(abs_ti, abs_t2) > 0:.
vshymanskyy commented 5 days ago

I think using ticks_diff is fine. Here's the direct example from documentation:

image

In case of aiodns, time.ticks_ms() and t are absolute, tout is a delta which should be OK

peterhinch commented 5 days ago

Where I suspect there is a problem is here:

tout = time.ticks_add(dt, 50)

dt is a delta (the result of ticks_diff). The docs for ticks_add state:

ticks parameter must be a direct result of call to ticks_ms(), ticks_us(), or ticks_cpu() functions...

The output of ticks_add is a ticks value, not a delta.

Given that both your args are deltas, and your intended result is a delta, I think it's OK to add them:

tout = dt + 50
vshymanskyy commented 5 days ago

Nice catch!

Currently getaddrinfo early-exits, but I'm thinking to keep the query process running in the background until the true timeout is reached. This way, more results will be cached and available for the application to use. The getaddrinfo will still early-exit on the first result, and the +50ms adjustment will probably be removed.