Server select timeout to prevent server to get stack because of client or network errors

[davidBar-On]

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

• Issues fixed (if any):
  A test request from an older client can permanently freeze the server #221

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

A proposed fix to prevent the the Server to be stack forever in case the client is stack of there is a network problem.

The main change is adding a timeout to the server's select(), so the fix should work as long as the select() timeout mechanism works properly. Two different timeouts are used:

1. When the server is idle waiting for connect request from a client, the timeout is specified by a new --idle-timeout parameter. The default is no timeout, to keep backward compatibility.
2. After receiving connect request from a client until the test ends the timeout is 2 minutes. I didn't add a parameter to set this timeout assuming that there is no scenario where the server will not receive anything from the client for more than 2 minutes.

If the select times out because of either of these timeouts, it will terminate the test and wait for a new test request.

A change I kept from the debugging phase is to add test number to the server message when waiting to new test request. E.g. "Server listening on 5201 (test #2)"

I don't have the full testing capabilities for such problems, but the fix works properly for all the scenarios I was able to test.

[bmah888]

Thanks for the PR! I generally like this code change, but I'm having some problems testing it (in particular, triggering the conditions that cause the . Could you tell me how you tested this to be sure it does what you want?

[davidBar-On]

This addition should go outside the HAVE_SSL conditional.

Fixed.

Could you tell me how you tested this to be sure it does what you want?

I tested the code by temporarily adding hard-coded sleep() after the client send with sleep time greater than the --idle-timeout value. That simulated the a client that got stack or disconnected after sending the first message and caused the server select to timeout.

I know such test may not take care of all possible cases. However, this test with success test cases covered all the code branches. Since when --idle-timeout is not set there is no change to the current behavior, then the risk is very low, as if worst comes to worst and there is an issue, users can just stop using this option.

[bmah888]

Thanks, I appreciate your low-risk approach. I'm just trying to figure out how to get the changes to actually make a difference. Here are a couple of things I tried, against a server invoked as iperf3 --server --idle-timeout 5, running on the same host (CentOS 7 VM):

% iperf3 --client localhost
(this was good...gave me a normal output...the only way I could tell the difference on the server was the adjusted output)

% iperf3 --client localhost
(then suspend the client process after a few seconds...I expected the server to close the connection after the timeout elapsed)

% nc localhost 5021
(didn't do anything...somehow I expected the server to close the connection after the 5 seconds I specified on the command-line)

So I'm trying to figure out basically how I can trigger the cases where the server will (with this change) behave better.

[davidBar-On]

Oops! My previous reply was incorrect. I confused myself between the idle and active cases .... Sorry for that.

The --idle-timeout is for the server idle mode, i.e. when it is waiting for new connection. E.g., with iperf3 --server --idle-timeout 5, when the server is idle the server will "restart" every 5 seconds ("restart" means closing connections the same as a test ended - not restart the process). Testing this is done by just not running a client and running the server with -d so it will print the "Server listening on ..." message every time it "restarts". The -d is needed since I thought that normally the serer "restart" in idle mode should be silent.

The temporary sleep was added only for the server active mode - when a client is connected. In this case the timeout is 2 minutes - defined by NO_MSG_RCVD_TIMEOUT, so the client sleep should be more then 2 minutes. Actually for testing I changed NO_MSG_RCVD_TIMEOUT to few seconds for faster testing and to make sure that other timeouts (e.g. TCP timeout) will not be triggered before this timeout.

Therefore, in the following cases:

% iperf3 --client localhost
(then suspend the client process after a few seconds...I expected the server to close the connection after the timeout elapsed)
% nc localhost 5021
(didn't do anything...somehow I expected the server to close the connection after the 5 seconds I specified on the command-line)

the connection should timeout after 2 minute.

[bmah888]

Thanks! I owe you some testing / review on the latest version of your PR.

[bmah888]

I understand now why I wasn't able to test in a meaningful way before. The problem was that there is already a server-side check that is used for timed tests (the default or with --time) that effectively limits the test on the server to the configured test time (default 10 seconds) plus 40 seconds. I was testing with the default 10-second tests, so that timer fired at 50 seconds into the test, much shorter than your 2-minute idle timer.

So if I do iperf3 --client localhost --bytes 100G and suspend the test in the middle, then wait around for 2 minutes, I get the 2-minute idle timeout message as expected.

Still fiddling around with this change but I now understand what it's doing, thanks.

[bmah888]

Apologies for taking so long to review this (and other) PRs you have submitted. Thanks for all your help!

This looks pretty good. I might do a few language nitpicks here and there after merging but that will just take me a few minutes I think. Also there is a slight problem when running the server with --json (it doesn't print any JSON output if the active test timeout happens)...but that's mostly due to a bug elsewhere in iperf3 that I will fix (and then tweak your code to track that fix).