sock_dns: implement DNS cache #17680
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benpicco
requested review from
miri64,
haukepetersen,
PeterKietzmann and
cgundogan
as code owners
github-actions
bot
added
Area: build system
JKRhb
reviewed
Feb 19, 2022
benpicco
force-pushed
the
sock_dns_cache
branch
from
046b54b to
e5b68e3
Compare
HendrikVE
suggested changes
Mar 10, 2022
There was a problem hiding this comment.
Nice one, I should have a look at the new PRs more often. I tested this PR with #17494 (without the topmost fixup commit) on esp32-wroom-32 so I can can query a specific IP versions. Works like a charm, but there are two small compilation issues for me.
benpicco
force-pushed
the
sock_dns_cache
branch
from
e5b68e3 to
414920b
Compare
|
Compiles now for all of my platforms. But I noticed something that does not seem quite right:
|
|
Uh good catch! |
Yep, can confirm. Let's see what murdock thinks about it. |
HendrikVE
added
the
CI: ready for build
benpicco
removed
the
CI: ready for build
|
There is still no test application so no compile test :( |
|
Whoops true 😄 The |
|
I now added a unit test for the module. |
|
I think the test should also cover that the cache eviction is working properly. The case of setting IPv6 for a domain but requesting IPv4 is already included, the mirrored case would also be nice to include. |
HendrikVE
reviewed
Apr 18, 2022
benpicco
added
the
CI: ready for build
|
Not necessarily needed to be done in this PR and wrt to #16705 I admit that I have an agenda ;-P, but I see some overlap between this cache and |
benpicco
force-pushed
the
sock_dns_cache
branch
2 times, most recently
from
b51ba34 to
48d4b17
Compare
benpicco
added
CI: ready for build
github-actions
bot
added
the
Area: CoAP
|
Happy happy API change and another user of |
|
About the long unittests: Can you reproduce this locally? Maybe for some reason there is some spin- or deadlock? |
|
I don't see the timeout when I run the test locally. |
Mh but the tests get stuck for a while for me when I run it locally... |
miri64
reviewed
Jul 14, 2022
| TEST_ASSERT_EQUAL_INT(0, sock_dns_cache_query("example.comm", &addr_out, AF_INET6)); | ||
| TEST_ASSERT_EQUAL_INT(0, sock_dns_cache_query("example.co", &addr_out, AF_INET6)); | ||
|
|
||
| ztimer_sleep(ZTIMER_MSEC, 2000); |
There was a problem hiding this comment.
Do we really have to have 2 seconds here? Can't you mock the now value or timeout some how to enforce a timeout?
There was a problem hiding this comment.
Alternatively, for testing you could make the clock resolution of sock_dns_cache_add()'s ttl configurable, and have it in milliseconds specifically for the tests.
Here's the full output from my local runsIt get's stuck in |
|
:-/ |
|
@benpicco: for your local tests: did you run all the unittests or just the |
|
Uh good point, when I run all the tests I also see the hang. |
|
The sleep does not hang when I use |
|
Would still prefer a solution that does not slow down the unittests by 2 seconds, but won't block. |
|
I think 2s isn't so bad in the grand scheme of things. |
This was referenced Jul 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
If a domain is used multiple times, RIOT will issue a new DNS query each time. This can add unwanted network overhead and latency, so caching DNS entries can be desirable.
If the cache is full, the first entry to expire will be evicted.
Testing procedure
The DNS cache is enabled with
Issues/PRs references