Remove calls to getaddrinfo_a #9229

1. 
   TheBlueMatt commented at 8:43 PM on November 27, 2016: contributor

   getaddrinfo_a has a nasty tendency to segfault internally in its background thread, on every version of glibc I tested, especially under helgrind.

   See https://sourceware.org/bugzilla/show_bug.cgi?id=20874

2. 
   TheBlueMatt commented at 8:43 PM on November 27, 2016: contributor

   This should probably be backported.

3. fanquake added the label Needs backport on Nov 27, 2016
4. 
   laanwj commented at 5:51 AM on November 28, 2016: member

   @theuni can you take a look here?

5. in configure.ac:None in e52c4cbddd outdated

    511 | @@ -512,7 +512,6 @@ if test x$TARGET_OS = xdarwin; then
    512 |  fi
    513 |  
    514 |  AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h stdio.h stdlib.h unistd.h strings.h sys/types.h sys/stat.h sys/select.h sys/prctl.h])
    515 | -AC_SEARCH_LIBS([getaddrinfo_a], [anl], [AC_DEFINE(HAVE_GETADDRINFO_A, 1, [Define this symbol if you have getaddrinfo_a])])
    516 |  AC_SEARCH_LIBS([inet_pton], [nsl resolv], [AC_DEFINE(HAVE_INET_PTON, 1, [Define this symbol if you have inet_pton])])
   

   ---

   ---

   

   laanwj commented at 7:05 AM on November 28, 2016:

   Interesting. It looks like this detection goes wrong on windows, and we only notice now because all uses were inside HAVE_GETADDRINFO_A. On windows you apparently have to use InetPton: https://msdn.microsoft.com/en-us/library/windows/desktop/cc805844(v=vs.85).aspx But this is only available on Vista and higher so the safest just to not use it on windows.

6. 
   laanwj commented at 9:49 AM on November 28, 2016: member

   On windows 32 bit (works correctly there):

   checking for library containing inet_pton... no

   On windows 64 bit:

   checking for library containing inet_pton... none required

   What is this "none required"? It detects the symbol in the C library? Maybe we need to check for include-ability as well?

   Edit: I checked and apparently the symbol is in libws2_32.a but not in any include files. The correct check here would be to try compiling a program that uses the function, not just a library linkage check. My autotools-fu doesn't go that far though.

7. 
   fanquake commented at 11:14 AM on November 28, 2016: member

   Looks like pretty much all the code being removed here was added in #4421

   re 'none required'. See here - https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Libraries.html "The result of this test is cached in the ac_cv_search_function variable as ‘none required’ if function is already available"

8. 
   theuni commented at 7:39 AM on December 1, 2016: member

   No real preference here. I'm just a few PRs away from removing the current resolve/connection code in favor of async lookups/connections.

   Given that our current connection model is very synchronous as-is, I've been curious for a while as to whether getaddrinfo_a is worth the trouble. So, concept ACK for nuke/backport.

9. jonasschnelli added this to the milestone 0.14.0 on Dec 1, 2016
10. Revert "Use async name resolving to improve net thread responsiveness"

    This reverts commit caf6150e9785da408f1e603ae70eae25b5202d98.
    
    getaddrinfo_a has a nasty tendency to segfault internally in its
    background thread, on every version of glibc I tested, especially
    under helgrind.
    
    See https://sourceware.org/bugzilla/show_bug.cgi?id=20874

    10ae7a7b23
11. TheBlueMatt force-pushed on Dec 1, 2016
12. 
    TheBlueMatt commented at 10:33 PM on December 1, 2016: contributor

    Switched to a wholesale revert of #4421 as discussed in meeting.

13. TheBlueMatt cross-referenced this on Dec 1, 2016 from issue Use async name resolving to improve net thread responsiveness by 4tar
14. 
    laanwj commented at 4:48 AM on December 2, 2016: member

    I've managed to reproduce the issue and posted to the sourceware thread. ACK 10ae7a7, prefer this as revert.

15. laanwj merged this on Dec 2, 2016
16. laanwj closed this on Dec 2, 2016

    ---

17. laanwj referenced this in commit c4d22f6eb2 on Dec 2, 2016
18. laanwj added this to the milestone 0.13.2 on Dec 2, 2016
19. laanwj removed this from the milestone 0.14.0 on Dec 2, 2016
20. laanwj referenced this in commit b172377857 on Dec 2, 2016
21. laanwj removed the label Needs backport on Dec 2, 2016
22. 
    laanwj commented at 4:51 AM on December 2, 2016: member

    pushed to 0.13 branch as b172377, removing backport tag

23. TheBlueMatt cross-referenced this on Apr 14, 2017 from issue Check interruptNet during dnsseed lookups by TheBlueMatt
24. TheBlueMatt cross-referenced this on Apr 14, 2017 from issue Frozen (for 15 minutes) on shutdown [dnsseed thread] by dooglus
25. codablock referenced this in commit e5ab072870 on Jan 16, 2018
26. codablock referenced this in commit 407c4577c7 on Jan 16, 2018
27. codablock referenced this in commit 62ae4e6449 on Jan 17, 2018
28. lateminer referenced this in commit 53e9126940 on Oct 22, 2018
29. andvgal referenced this in commit 9b4126b931 on Jan 6, 2019
30. CryptoCentric referenced this in commit bd538f4453 on Feb 25, 2019
31. Nikolay-Po cross-referenced this on Sep 3, 2019 from issue ThreadDNSAddressSeed hangs on sk_wait_data and doesn't stop on exit by Nikolay-Po
32. furszy cross-referenced this on Dec 11, 2020 from issue [Net] Socks5 netbase back ports, less scary failures. by furszy
33. random-zebra referenced this in commit d9cbbad1dc on Dec 14, 2020
34. bitcoin locked this on Sep 8, 2021

Contributors

TheBlueMattlaanwjfanquaketheuni

Milestone
0.13.2