-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add control connection keepalive arguments #1423
base: master
Are you sure you want to change the base?
Add control connection keepalive arguments #1423
Conversation
@ankitg12, I reply to your comment here, as building the change is using the code of this PR.
As this PR is not merged (yet) into the main code, there are probably no available binaries. You can try building it yourself. In general, the process is (under Linux): git clone https://github.com/davidBar-On/iperf.git --branch issue-812-835-control-connection-keepalive
cd iperf
./bootstrap.sh
./confoigure
make The iperf3 executable will be Note that the PR is not fully tested. If you were able to build and run iperf3 successfully and you find problems with the implementation of the new option, please report them in this PR. Thanks. |
I guess this is probably obvious, but I had to add I can confirm with KEEPIDLE = Modify the interval of the keepalive I would be very happy to see this change merge into the master branch! |
@lbsou, Which OS are you using? Can you check if and where |
Hi! I tried on the following platforms :
The option is available on both system :
I was able to compile and run the following code:
Returned : 4 |
Hi, thanks for the info! Hopefully I now understand the problem. When (if...) the PR will be merged into the mainline, Can you try running Now run Was this successful? If it did, I will add the above instructions to the PR description |
Thank you David, that was it! I did Any idea when this change will be merged? |
@lbsou, I have updated the PR description. Thanks for your efforts and time for clarifying this issue (and helping me to better understand the configuration process ...). I am not from the team that maintains iperf3, so I don't know if and when this PR will be merged into the mainline. |
Thank you David! @bmah888 Can we be of any further help for this PR to be merged? Thanks! |
Hi @davidBar-On ! I did some more testing and I can activate keepalive only from client to server, not from server to client. Server :
Client :
This means the server might not notice if the control channel dies (aggressive nat gateway session timeout?) and then the server is unreachable forever (in case of a bidir communication, the server continues sending udp packet indefinitely.). |
4c4139d
to
266ca8d
Compare
Hi @lbsou, thanks for testing and finding this issue. However, when evaluating your comment I realized that it is better that the keepalive option will be available for both the client and server. I enhanced the solution and the option should work now on both sides. |
266ca8d
to
40345dc
Compare
Hi @lbsou, thanks for testing and finding this issue. Actually, setting the keepalive only on the client side was intentional, as the client is initiating the control stream, but I forgot to limit the option for the client. However, when evaluating your comment I realized that it is better that the keepalive option will be available for both the client and the server. I enhanced the solution and the option should work now on both sides. |
Thanks you very much @davidBar-On, may the gods of merging be with us! |
We might look into testing and merging this soon. We're interested in the changes. In the meantime, if you could update the man page for the new command line argument that would be great, otherwise we'll probably get to it once multi-threading is merged. |
2d0532f
to
9d73cee
Compare
Version of iperf3 (or development branch, such as
master
or3.1-STABLE
) to which this pull request applies:master
Issues fixed (if any):
Cannot test long UDP sessions #812, intermittent connection #835
Brief description of code changes (suitable for use as a commit message):
Adding Control-connection TCP Keepalive arguments:
--cntl-ka [<KEEPIDLE>/<KEEPINTV>/<KEEPCNT>]
.As suggested in the resolved issues, setting the keepalive period (
TCP_KEEPIDLE
) resolves #812 by allowing to keep the control connection alive longer than the firewall timeout, and resolves #835 by allowing the server to terminate inactive connections after shorter time.Added
HAVE_TCP_KEEPALIVE
, based onTCP_KEEPIDLE
availability, as I am not sure if and which OSs support the TCP keepalive. In principle, there could be a separated "HAVE" for the socket (not TCP)SO_KEEPALIVE
, but I assume this is not necessary.(Note based on the discussion below starting with this comment: to use this PR before it is merged into main line,
autoconf
andautoheader
should be run before./configure
. Ifautoconf
fails with error that version 2.71 should be used, runautoconf -V
to get its version and change "2.71" inconfigure.ac
to that version.)I was not able to fully test the code, as I was not able to test the keepalive retries.
Also, for some reason, in Windows WSL2 it seems that the keepalive period (
TCP_KEEPIDLE
) must be greater than the retry interval (TCP_KEEPINTV
). Otherwise, the keepalive is sent only once. As I couldn't test the retries, I assumed that it may even be that the keepalive period must be greater than the retry period (TCP_KEEPINTV
*TCP_KEEPCNT
). Therefore, in case this condition is not met and only the keepalive period was set, I added code that reduces the retry interval accordingly.