Use async name resolving to improve net thread responsiveness #4259

1. 
   4tar commented at 2:34 pm on May 30, 2014: contributor

   This is the re-submit of PR 4215, just combine all several commits in that PR into one. The previous discussion could be checked here: #4215.

   In the LookupIntern(), things changed are:

   1. Call getaddrinfo_a() instead of getaddrinfo() if available, the former is a sync version of the latter;
   2. Try using inet_pton()/inet_addr() to convert the input text to a network addr structure at first, if success the extra name resolving thread inside getaddrinfo_a() could be avoided;
   3. An interruption point added in the waiting loop for return from getaddrinfo_a(), which completes the improve for thread responsiveness.

   A easy way to see the effect is to kick off a ‘bitcoind stop’ immediately after ‘bitcoind -daemon’, before the change it would take several, or even tens of, minutes on a bad network situation to wait for the running bitcoind to exit, now it costs only seconds.

   Signed-off-by: Huang Le 4tarhl@gmail.com

2. Use async name resolving to improve net thread responsiveness

   In the LookupIntern(), things changed are:
     1. Call getaddrinfo_a() instead of getaddrinfo() if available, the former is a sync version of the latter;
     2. Try using inet_pton()/inet_addr() to convert the input text to a network addr structure at first, if success the extra name resolving thread inside the async getaddrinfo_a() could be avoided;
     3. An inturrption point added in the waiting loop for return from getaddrinfo_a(), which completes the improve for thread responsiveness.
   
   Signed-off-by: Huang Le <4tarhl@gmail.com>

   b28a3bee9f
3. in src/netbase.cpp: in b28a3bee9f

   82@@ -71,9 +83,30 @@ bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsign
   83         }
   84     }
   85 
   86+#ifdef HAVE_GETADDRINFO_A
   

   ---

   ---

   

   sipa commented at 9:47 am on May 31, 2014:

   Does this section rely on getaddrinfo_a?

   ---

   

   4tar commented at 4:10 am on June 1, 2014:

   @sipa No, it doesn’t. But It would be unnecessary if getaddrinfo_a() is unavailable, since placing a call to inet_pton()/inet_addr() first is to avoid the extra thread inside getaddrinfo_a(), else getaddrinfo() can take care of everything.
4. 
   jgarzik commented at 6:58 pm on June 4, 2014: contributor

   Seems straightforward enough, but simplistic. getaddrinfo_a() allows for async callbacks, eliminating the need for any wait loop.
5. 
   laanwj commented at 7:09 pm on June 4, 2014: member

   @jgarzik But the point here is only to make the DNS lookup interruptible, I think getting a hand-rolled callback-based system to cooperate with boost threads will be complicated. Might as well switch to boost::asio which implements all this already, and in a cross-platform way.
6. 
   4tar commented at 4:34 am on June 5, 2014: contributor

   @laanwj Yes using Asio sounds a better way, let me test it locally and report back.
7. 
   laanwj commented at 4:42 am on June 5, 2014: member

   @4tar Though you may run into other, new problems there, as the rest of the net/netbase code doesn’t use boost::asio, and you have to convert addresses, BoostAsioToCNetAddr which I introduced with the subnet matching may come in handy here, see ee21912 and 21bf3d2.
8. 
   gmaxwell commented at 8:02 am on June 5, 2014: contributor

   Boost ASIO is pretty awful in my experience, it’s buggy, non-performant (it spends a bunch of time thrashing the allocator), results in random deadlocks in even moderately complex code, and its virtually impossible to debug.

   I’m not clear on what needs to change here wrt DNS resolution— we’ve already move the dnsseeds into a thread. If we’re doing anything performance critical depending on DNS that in and of itself might be the mistake. :)

9. 
   laanwj commented at 8:09 am on June 5, 2014: member

   @gmaxwell This is not about performance, it doesn’t matter at this particular point. The pull just aims to make shutdown more reliable. getaddrinfo is not interruptible, which means that shutdown of bitcoind will wait which may take a long time.

   If boost::asio is so awful (which I sort-of agree on, at least the debugging aspect), any points for a better network library we could switch to for at least RPC? I’m sure this is something that has been done and re-done over the ages and it serves us no purpose to try to reinvent everything.

10. 
    4tar commented at 6:34 pm on June 6, 2014: contributor

    @laanwj Wow, didn’t notice your previous comment and had done it again… Yes, this method is pretty handy for this purpose, I’d like suggest to put it as another CNetAddr constructor instead of a independent function. Actually I’ve implement it so, will submit a PR including this change soon, and I have to say that boost::asio was discovered to be not perfect for this case…
11. 
    BitcoinPullTester commented at 6:00 am on June 23, 2014: none

    Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p4259_b28a3bee9f8250e083b715a98df7dd7208db0f37/ for test log.

    This pull does not merge cleanly onto current master This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

12. 4tar closed this on Jun 26, 2014

    ---

13. 4tar deleted the branch on Jun 26, 2014
14. MarcoFalke locked this on Sep 8, 2021

Contributors
4tar sipa jgarzik laanwj gmaxwell BitcoinPullTester