Use async name resolving to improve net thread responsiveness #4259
Conversation
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>
| @@ -71,9 +83,30 @@ bool static LookupIntern(const char *pszName, std::vector<CNetAddr>& vIP, unsign | |||
| } | |||
| } | |||
|
|
|||
| #ifdef HAVE_GETADDRINFO_A | |||
There was a problem hiding this comment.
Does this section rely on getaddrinfo_a?
There was a problem hiding this comment.
@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.
|
Seems straightforward enough, but simplistic. getaddrinfo_a() allows for async callbacks, eliminating the need for any wait loop. |
|
@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. |
|
@laanwj Yes using Asio sounds a better way, let me test it locally and report back. |
|
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. :) |
|
@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. |
|
@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... |
|
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 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:
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