Wrap getaddrinfo to try more than once on Windows.

[madbrain76]

PLEASE NOTE the following text from the iperf3 license. Submitting a
pull request to the iperf3 repository constitutes "[making]
Enhancements available...publicly":

You are under no obligation whatsoever to provide any bug fixes, patches, or
upgrades to the features, functionality or performance of the source code
("Enhancements") to anyone; however, if you choose to make your Enhancements
available either publicly, or directly to Lawrence Berkeley National
Laboratory, without imposing a separate written license agreement for such
Enhancements, then you hereby grant the following license: a non-exclusive,
royalty-free perpetual license to install, use, modify, prepare derivative
works, incorporate into other computer software, distribute, and sublicense
such enhancements or derivative works thereof, in binary and source code form.

The complete iperf3 license is available in the LICENSE file in the
top directory of the iperf3 source tree.

• Version of iperf3 (or development branch, such as master or
  3.1-STABLE) to which this pull request applies:

• Issues fixed (if any):

• Brief description of code changes (suitable for use as a commit message):

[davidBar-On]

Error should be either printer only if debug option was set or using iperf_err() (to properly handle the JSON output case). Suggest to leave the decision to the iperf3 maintainers (before merging the change).

In both cases, test should be passed as a parameter to the function and iperf.h should be included. In case iperf_err is used, also iperf_api.h should be included

If printed as debug message, suggest to use the newly added debug level option:

if (test->debug_level >= DEBUG_LEVEL_ERROR))
    fprintf(...);

[madbrain76]

David, thanks for the feedback. Obviously, I wrote my PR before the option was added. I would prefer for this message to be printed unconditionally if getaddrinfo fails intermittently, regardless of the new debug option. The rationale is that with the loop in place, a fatal error would not be seen anymore. And without being aware of the intermittent failure behind the scene, nobody would think to add -d. The intermittent failure seems rare enough that I wanted anyone with a affected system to see the message. Requiring -d would significantly decrease the odds anyone would ever know about the problem. The wording of the message could be changed, though.

I checked the source to see where debug_level was being set. The iperf_test structure that contains debug_level isn't available in the scope of most of the calls to getaddrinfo . A pointer to it would need to be propagated across the stack. getaddrinfo itself of course, being a low-level call, doesn't take this object as an argument. I am reluctant to change the prototype for getaddrinfo . I could make it a wrapper on all platforms, instead of having the platform #ifdef, though.

iperf_test appears to contain a global configuration state. And there is only ever a single instance, created in main() with a call to iperf_new_test. As long as that remains the case, it strikes me as a bit of busy work to pass this object around the stack down everywhere, not just to iperf_getaddrinfo. I don't want to address that in this PR.

[davidBar-On]

I would prefer for this message to be printed unconditionally if getaddrinfo fails intermittently, regardless of the new debug option.

The new debug option is just the ability to set a debug level. Using if (test->debug) always existed, but it still needs the use of iperf_test.

The rationale is that with the loop in place, a fatal error would not be seen anymore. And without being aware of the intermittent failure behind the scene, nobody would think to add -d.

I agree. In this case still iperf_err() should be used to log the error, to properly handle JSON output (in case -J was set), which also needs the use of iperf_test.

A pointer to it would need to be propagated across the stack. getaddrinfo itself of course, being a low-level call, doesn't take this object as an argument. I am reluctant to change the prototype for getaddrinfo . .....

I agree that adding iperf_test to getaddrinfo and to net.c in general is a problem. I therefore suggest to leave the decision to the iperf3 maintainers whether to propagate iperf_test down to net.c, not log the error in getaddrinfo, add a wrapper or any of the other approaches you suggested.

[davidBar-On]

@bmah888, I believe this PR should be merged into the mainline, as it seems to be important for Windows users. It fixes issue #1314 and probably also #1201.

Although I did not see similar getaddrifo issues for non Windows OSs, the PR may be considered for all all OSs. The root cause of the problem is not clear, and it may be related only to Cigwin and/or Windows. However, since the change is just retrying few time getaddrinfo in case it fails, the only harm in case of permanent getaddrinfo problem is that it will take more time for the client to fail (and the retries count may be smaller than 10).

[bmah888]

@davidBar-On @madbrain76 : Sorry for the delay! I understand this PR seems to fix an actual, observed problem when running iperf3 under Cygwin/Windows.

Nevertheless, this isn't a platform that we officially support, and I'm reluctant to commit what's basically a hack around an OS bug (it seems) and have that code change affecting the behavior of our officially-supported platforms. I'm going to claim that if this was an issue on Linux, FreeBSD, or macOS, we likely would have encountered this already.

I'm not inclined to take this patch in its current form, however I might be willing to take a PR in this area if it involved a conditional compilation, such that the code change only applies to Windows and has no effect on our supported systems. How's that sound?