net: call getaddrinfo() in detachable thread to prevent stalling #27557

pinheadmz · 2023-05-02T18:28:32Z

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:

We could make the polling loop interruptible but I'm not sure the best approach to that, since these functions don't live in a class with a flag like interruptNet

DrahtBot · 2023-05-02T18:28:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

fanquake · 2023-05-03T09:03:01Z

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).

src/test/netbase_tests.cpp

dergoegge · 2023-05-03T10:55:13Z

src/netbase.cpp

+        std::thread thread(AsyncGetAddrInfo, &req);
+        int checks{0};
+        while (true)
+        {
+            {
+                LOCK(gai_mutex);
+                if (req.complete) break;
+            }
+            // Check every 100ms for 2 seconds
+            if (++checks > 20) break;
+            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+        }
+        thread.detach();


nit: Maybe use https://en.cppreference.com/w/cpp/thread/future/wait_until instead?

furszy · 2023-05-03T13:30:09Z

src/netbase.cpp

+    if (allow_lookup) {
+        // Execute the DNS lookup in another thread and check it periodically for completion.
+        // After sufficiently waiting, abandon the thread entirely. The user's system
+        // may wait forever for a DNS response that never returns.
+        std::thread thread(AsyncGetAddrInfo, &req);
+        int checks{0};
+        while (true)
+        {
+            {
+                LOCK(gai_mutex);
+                if (req.complete) break;
+            }
+            // Check every 100ms for 2 seconds
+            if (++checks > 20) break;
+            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+        }
+        thread.detach();


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 (f448668) 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.

pinheadmz · 2023-05-03T19:15:56Z

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.

pinheadmz · 2023-05-05T17:16:48Z

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.

pinheadmz · 2023-05-05T18:37:47Z

...got thread sanitizer errors running locally on main when my local DNS is black-holed. Fixed that with a shared pointer.

src/netbase.cpp

pinheadmz · 2023-06-05T18:29:31Z

Needs rebase?

ty. 🤞

luke-jr · 2023-07-01T22:08:06Z

src/netbase.cpp

+    if (allow_lookup) {
+        // Execute the DNS lookup in another thread and check it periodically for completion.
+        // After sufficiently waiting, abandon the thread entirely. The user's system
+        // 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.

luke-jr · 2023-07-01T22:17:12Z

src/test/netbase_tests.cpp

+{
+    // Non-numeric lookup that does not require actual DNS service
+    BOOST_CHECK(LookupHost("localhost", /*fAllowLookup=*/true).has_value());
+    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)

DrahtBot added the P2P label May 2, 2023

pinheadmz mentioned this pull request May 2, 2023

net: use interruptible async getaddrinfo wrapper from libevent for DNS #27505

Closed

3 tasks

DrahtBot added the CI failed label May 2, 2023

This was referenced May 2, 2023

net: support unix domain sockets for -proxy and -onion #27375

Merged

Handle CJDNS from LookupSubNet() #27071

Merged

dergoegge reviewed May 3, 2023

View reviewed changes

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

dergoegge reviewed May 3, 2023

View reviewed changes

furszy reviewed May 3, 2023

View reviewed changes

pinheadmz force-pushed the async-getaddrinfo branch 6 times, most recently from 56a3cbd to 4150942 Compare May 3, 2023 17:57

DrahtBot removed the CI failed label May 3, 2023

pinheadmz force-pushed the async-getaddrinfo branch from 4150942 to 1d57811 Compare May 5, 2023 17:14

pinheadmz force-pushed the async-getaddrinfo branch from 1d57811 to c292288 Compare May 5, 2023 18:34

DrahtBot added CI failed and removed CI failed labels May 5, 2023

pinheadmz commented May 11, 2023

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

pinheadmz force-pushed the async-getaddrinfo branch from c292288 to 3db04cc Compare May 12, 2023 17:19

DrahtBot mentioned this pull request May 17, 2023

ZMQ: Support UNIX domain sockets #27679

Merged

DrahtBot added the CI failed label Jun 4, 2023

pinheadmz force-pushed the async-getaddrinfo branch from 3db04cc to c3ccbb2 Compare June 5, 2023 18:29

pinheadmz force-pushed the async-getaddrinfo branch from c3ccbb2 to 2910e59 Compare June 5, 2023 18:37

DrahtBot removed the CI failed label Jun 5, 2023

luke-jr reviewed Jul 1, 2023

View reviewed changes

net: call getaddrinfo() in detachable thread to prevent stalling

be52064

pinheadmz force-pushed the async-getaddrinfo branch from 2910e59 to be52064 Compare July 5, 2023 19:10

achow101 requested review from theuni and fanquake September 20, 2023 16:59

DrahtBot added the Needs rebase label Oct 19, 2023

DrahtBot removed the Needs rebase label Nov 7, 2023

DrahtBot mentioned this pull request Nov 7, 2023

log: torcontrol opt checks #28780

Closed

DrahtBot added the CI failed label Jan 13, 2024

achow101 closed this Apr 9, 2024

bitcoin locked and limited conversation to collaborators Apr 9, 2025

net: call getaddrinfo() in detachable thread to prevent stalling #27557

net: call getaddrinfo() in detachable thread to prevent stalling #27557

Uh oh!

Conversation

pinheadmz commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dergoegge May 3, 2023

Choose a reason for hiding this comment

Uh oh!

furszy May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dergoegge May 4, 2023

Choose a reason for hiding this comment

Uh oh!

furszy May 5, 2023

Choose a reason for hiding this comment

Uh oh!

dergoegge May 5, 2023

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented May 5, 2023

Uh oh!

pinheadmz commented May 5, 2023

Uh oh!

Uh oh!

pinheadmz commented Jun 5, 2023

Uh oh!

luke-jr Jul 1, 2023

Choose a reason for hiding this comment

Uh oh!

pinheadmz Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr Jul 1, 2023

Choose a reason for hiding this comment

Uh oh!

pinheadmz Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pinheadmz commented May 2, 2023 •

edited

Loading

DrahtBot commented May 2, 2023 •

edited

Loading

fanquake commented May 3, 2023 •

edited

Loading

furszy May 3, 2023 •

edited

Loading

pinheadmz commented May 3, 2023 •

edited

Loading

pinheadmz Jul 5, 2023 •

edited

Loading