Skip to content
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

tcpkeepalive: add CURLOPT_TCP_KEEPCNT for libcurl and keepalive-cnt for curl #13885

Closed
wants to merge 1 commit into from

Conversation

panjf2000
Copy link
Contributor

@panjf2000 panjf2000 commented Jun 5, 2024

Customizing TCP_KEEPIDLE and TCP_KEEPINTVL for curl has been supported since v7.18.0, with TCP_KEEPCNT missing all the time. TCP_KEEPIDLE, TCP_KEEPINTVL, and TCP_KEEPCNT are not included in the POSIX standard for system call setsockopts(), yet while these three socket options are widely available on most Unix-like systems and Windows.

I reckon that curl should support setting TCP_KEEPCNT and use a default value if unspecified across all platforms based off of the following justification:

  • The system default value of TCP_KEEPCNT varies across different platforms that support this option: the maximum number of keep-alive probes sent before dropping the connection is 8 on *BSD/macOS/AIX, 9 on Linux/AIX(old), 5/10 on Windows (5 on Windows Server 2003, Windows XP, and Windows 2000, 10 on Windows Vista and later), or even more unorthodox, undefined default value on Solaris/illumos, and so on, which brings forth the inconsistency of curl on various platforms when it comes to the behaviors of TCP keep-alive mechanism. Setting up a default value for TCP_KEEPCNT regardless of specific platforms will consolidate the behaviors of TCP keep-alive.

  • The requirement of precisely tuning the TCP keep-alive behaviors for some purposes: 1) aggressive default values of TCP keep-alive can result in excessive CPU consumption and drain the battery fast if it's on mobile devices (that are tolerant with dead connections), people can alleviate it by tuning up TCP_KEEPINTVL and tuning down TCP_KEEPCNT, 2) on the contrary, for some applications that are aliveness-sensitive about the remote peers, people need to tune down both the TCP_KEEPINTVL and TCP_KEEPCNT to detect the dead peers ASAP.

Thus, the support of the full TCP keep-alive mechanism (all three options) enables users to take control of the TCP keep-alive mechanism more finely and build more superior applications based on curl/liburl.

References

Mac OS

In earlier versions, macOS only supported setting TCP_KEEPALIVE (the equivalent of TCP_KEEPIDLE on other platforms), but since macOS 10.8 it has supported TCP_KEEPINTVL and TCP_KEEPCNT.

Check out this mailing list and the source code for more details.

Solaris

Solaris claimed it supported the TCP keep-alive mechanism, but TCP_KEEPIDLE, TCP_KEEPINTVL, and TCP_KEEPCNT were not available on Solaris until the latest version 11.4. Therefore, we need to simulate the TCP keep-alive mechanism on other platforms via TCP_KEEPALIVE_THRESHOLD + TCP_KEEPALIVE_ABORT_THRESHOLD.

Relevant issues

#8564
#10318

@bagder bagder added the feature-window A merge of this requires an open feature window label Jun 5, 2024
@panjf2000 panjf2000 changed the title tcpkeepalive: add CURLOPT_TCP_KEEPCNT for libcurl and keepalive-probes for curl tcpkeepalive: add CURLOPT_TCP_KEEPCNT for libcurl and keepalive-cnt for curl Jun 5, 2024
@panjf2000
Copy link
Contributor Author

Hah, this CI failure seems to be caused by this change? I'm not sure, I wonder if changing the option from 1 to 9 or 10 would fix it.

@bagder
Copy link
Member

bagder commented Jun 5, 2024

Hah, this CI failure seems to be caused by this change?

Not necessarily, it might also just be a flaky test/build...

@dfandrich
Copy link
Contributor

dfandrich commented Jun 5, 2024 via email

@panjf2000
Copy link
Contributor Author

panjf2000 commented Jun 6, 2024

My question would be when users run curl on a new platform, should they expect
to see platform default behaviour or curl default behaviour? I would venture
that platform default behavior is more to be expected. These are low-level
details that are expected to be different on different OSes; that's what makes
different OSes different, after all.

Not all system default behaviors are rational, sometimes they're even lame. Take the TCP keep-alive for instance, most of the platforms use 2 hours as the default value of TCP_KEEPIDLE, which makes this whole mechanism useless because it would take you over 2 hours to get notified that an idle connection was dead (two hours ago). That's why most of the libraries that utilize TCP keep-alive don't use its default values defined by the OSs, but change those values of options accordingly to make it actually pragmatic. And that's also what curl has been doing: set TCP_KEEPIDLE and TCP_KEEPINTVL to 60s by default when keep-alive is enabled but these two options weren't specified.

why stop at
trying to make TCP keepalive behaviour the same across platforms? Why not make
ephemeral port ranges the same, or socket buffer sizes the same, etc., etc.?

We should only try to consolidate the behaviors for libraries that aim to be cross-platform and portable (and I believe this has also been curl's goal for years, maybe from the day it was created). We don't compulsively need to line up all system configurations under the hood when they don't have an evident impact on the behaviors of applications, like the socket buffer you referred to, different default values for socket buffers on various platforms rarely led people to spot the different behaviors during networking, moreover, most of the platforms use the dynamic socket buffers instead of static ones, which makes the consolidation of that redundant. But for TCP keep-alive options, people will effectively see the consequence due to the inconsistent default values across platforms, because it's a time-sensitive mechanism.

@panjf2000
Copy link
Contributor Author

When can we get the review started? Would you take a glance at this PR? @bagder

@bagder bagder closed this in b77d627 Jun 12, 2024
@bagder
Copy link
Member

bagder commented Jun 12, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmdline tool feature-window A merge of this requires an open feature window libcurl API tests
Development

Successfully merging this pull request may close these issues.

None yet

3 participants