The library call getaddrinfo() has no internal timeout and depending on the user's system may wait indefinitely for a response when looking up a hostname in the DNS. By making that call in a separate thread, we can abandon it completely after some timeout (currently in this PR, 2 seconds).
TODO:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
The unit test I added makes a live DNS request for nic.com that test will fail the platform has no DNS
Or just anytime you don't have an internet connection? We're not going to add "internet connectivity" as a requirement to run the unit tests (putting aside all other potential issues with doing this).
157 | @@ -158,6 +158,19 @@ BOOST_AUTO_TEST_CASE(embedded_test) 158 | BOOST_CHECK_EQUAL(addr1.ToStringAddr(), addr2.ToStringAddr()); 159 | } 160 | 161 | +BOOST_AUTO_TEST_CASE(dns_lookup) 162 | +{ 163 | + CNetAddr addr; 164 | + bool ret = LookupHost("nic.com", addr, /*fAllowLookup=*/true); 165 | + BOOST_CHECK(ret); 166 | + BOOST_CHECK(addr.IsRoutable());
Making actual requests from the unit tests is not great.
If you want to add a unit test, you could introduce a way to mock getaddrinfo or AsyncGetAddrInfo so that you can better test our code in all the relevant scenarios (i.e. getaddrinfo errors/blocking etc.).
115 | + } 116 | + // Check every 100ms for 2 seconds 117 | + if (++checks > 20) break; 118 | + std::this_thread::sleep_for(std::chrono::milliseconds(100)); 119 | + } 120 | + thread.detach();
nit: Maybe use https://en.cppreference.com/w/cpp/thread/future/wait_until instead?
Performance wise, create a new thread per lookup is quite bad. Should use a workers pool instead.
I have implemented a generic one for #26966 (https://github.com/bitcoin/bitcoin/pull/26966/commits/f448668f473675915fcebf0e1bd3d73dcb1881af) that could also use here. It also support callback futures so you can remove the costly active wait as well.
Give it a look and if you like it, could decouple it into a standalone PR so we could start using it in both PRs.
Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).
Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.
Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).
k, getaddrinfo doesn't have a timeout and getaddrinfo_a seems to have a segfault. Cool..
Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.
I was thinking more about context switching. Isn't this used for OpenNetworkConnection too? So, it would create a new thread for every connection attempt? (unless the lookups are numeric, which guess that is the rule here..).
And, following the worst case scenario, what if the user call addnode a good number of times? we would get a lot of detached threads? (unless we set a limit for the amount of lookups that can be resolved in parallel).
k, getaddrinfo doesn't have a timeout and getaddrinfo_a seems to have a segfault. Cool..
Yea one would think that making DNS requests in c++ would be a solved problem.
Anyway, I'm also not sure about the approach here. The original issue was that we would block indefinitely while joining the DNS thread on shutdown (only happens if the user's system is broken I think?). I suggested to detach the dns thread if it doesn't join in a reasonable amount of time (would be fine, the OS will clean the thread up since we are about to exit anyway).
The approach here risks having threads hang around for the lifetime of a node, which seems worse than the occasional shutdown hang on broken systems.
Still waiting for CI to finish but I removed the need for internet access by looking up the name "localhost" instead. That is a non-numeric lookup that getaddrinfo() can do without the actual DNS. I think I also fixed the ipv6 test issue:
(run in docker with no ipv6)
test/netbase_tests.cpp(161): Entering test case "dns_lookup"
test/netbase_tests.cpp(165): info: check LookupHost("localhost", addr, true) has passed
test/netbase_tests.cpp(169): info: check Lookup("127.0.0.1:12345", serv, 9050, true) has passed
test/netbase_tests.cpp(176): warning: in "netbase_tests/dns_lookup": Skipping IPv6 check
test/netbase_tests.cpp(161): Leaving test case "dns_lookup"; testing time: 217012us
I'm interested in following the worker pool idea to make these requests async as opposed to new threads.
added call to thread.join() in the "happy path" so the only threads we should be abandoning are those that time out. Also setup the unit test to stub in a black hole for getaddressinfo to ensure the 2-second timeout is enforced. On my machine at least with thread and memory sanitizers I'm not getting any errors for the abandoned thread. I'm going to try some more experiments with a mainnet node as well.
...got thread sanitizer errors running locally on main when my local DNS is black-holed. Fixed that with a shared pointer.
39 | @@ -40,7 +40,9 @@ bool fNameLookup = DEFAULT_NAME_LOOKUP; 40 | int g_socks5_recv_timeout = 20 * 1000; 41 | static std::atomic<bool> interruptSocks5Recv(false); 42 | 43 | -std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_lookup) 44 | +static GlobalMutex gai_mutex;
TIL I can probably replace this with std::atomic_bool
Needs rebase?
ty. 🤞
97 | + std::shared_ptr<GAIRequest> req = std::make_shared<struct GAIRequest>(allow_lookup, name, &resolved_addresses); 98 | + 99 | + if (allow_lookup) { 100 | + // Execute the DNS lookup in another thread and check it periodically for completion. 101 | + // After sufficiently waiting, abandon the thread entirely. The user's system 102 | + // may wait forever for a DNS response that never returns.
Probably not good to ignore it never returning... it's leaking resources. Maybe kill it and/or tell the user somehow? :/
I added a log message but not sure what else to do. The problem is that getaddrinfo() is blocking and getaddrinfo_a() segfaults 😠As long as the DNS request resolves eventually the thread will close, even if bitcoind has given up and detached the thread. If getaddrinfo() hangs forever then yes you're right that is a leak, but the user also has a serious networking issue.
155 | @@ -156,6 +156,42 @@ BOOST_AUTO_TEST_CASE(embedded_test) 156 | BOOST_CHECK_EQUAL(addr1.ToStringAddr(), addr2.ToStringAddr()); 157 | } 158 | 159 | +BOOST_AUTO_TEST_CASE(dns_lookup) 160 | +{ 161 | + // Non-numeric lookup that does not require actual DNS service 162 | + BOOST_CHECK(LookupHost("localhost", /*fAllowLookup=*/true).has_value()); 163 | + BOOST_CHECK(!LookupHost("fail.doesnotexist", /*fAllowLookup=*/true).has_value());
Won't this still make an externally-visible DNS request?
Yep you are absoutley right, I'll remove fail.doesnotexist (localhost will NOT make an external request)
