Remove calls to getaddrinfo_a #9229

1. 
   TheBlueMatt commented at 8:43 pm on November 27, 2016: member

   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: member

   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: 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: member

    Switched to a wholesale revert of #4421 as discussed in meeting.
13. 
    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.
14. laanwj merged this on Dec 2, 2016
15. laanwj closed this on Dec 2, 2016

    ---

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

    pushed to 0.13 branch as b172377, removing backport tag
22. codablock referenced this in commit e5ab072870 on Jan 16, 2018
23. codablock referenced this in commit 407c4577c7 on Jan 16, 2018
24. codablock referenced this in commit 62ae4e6449 on Jan 17, 2018
25. lateminer referenced this in commit 53e9126940 on Oct 22, 2018
26. andvgal referenced this in commit 9b4126b931 on Jan 6, 2019
27. CryptoCentric referenced this in commit bd538f4453 on Feb 25, 2019
28. random-zebra referenced this in commit d9cbbad1dc on Dec 14, 2020
29. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt laanwj fanquake theuni

Milestone
0.13.2