Add DNS resolution timeout for realm list updates #3198
Conversation
src/shared/IO/Networking/DNS.cpp
Outdated
| addrinfo* dnsResult = nullptr; | ||
| if (::getaddrinfo(domainName.c_str(), nullptr, &hints, &dnsResult) != 0) | ||
| // Worker thread that performs the DNS resolution | ||
| std::thread resolverThread([state, domainName, type]() |
There was a problem hiding this comment.
I am not sure if this is good practice. The other solution would be to use our IoContext and then ::GetAddrInfoExW
There was a problem hiding this comment.
btw we dont use std::thread to spawn threads. Prefer to use IO::Multithreading::CreateThread
There was a problem hiding this comment.
Thanks for the feedback! Went with the lazy IO::Multithreading::CreateThread solution for now - need to look into ::GetAddrInfoExW, but this would mean platform specific implemenations, correct?
There was a problem hiding this comment.
Yeah, its OS specific. In linux/macos it would be getaddrinfo_a.
Although your current solution works, it feels wrong to create a heavy std::thread for just a simple IO task and letting a timeout run in the main thread.
And what happens if a DNS requests gets stuck forever and never resolves?
MAX_PENDING_DNS_RESOLUTIONS would be reached and is denying further requests that might work.
With GetAddrInfoExW/getaddrinfo_a you don't need to spawn a new thread and correctly cancel timed'out requests.
| MANGOS_ASSERT(type == IpAddress::Type::IPv4); // TODO: this function is only tested with IPv4. `inet_ntop` will fail | ||
| // Limit concurrent pending DNS resolutions to prevent thread accumulation on DNS failures | ||
| std::atomic<int> g_pendingDnsResolutions{0}; | ||
| constexpr int MAX_PENDING_DNS_RESOLUTIONS = 10; |
|
Have you experienced a blocking DNS requests on your system or what's the reason for this PR? |
2 participants
🍰 Pullrequest
Adds configurable DNS resolution timeout to prevent the realm server from hanging when DNS servers are unreachable or slow to respond.
Proof
Issues
How2Test
Todo / Checklist