Use WSD-config or system's maximum concurrent TCP connections for net::Defaults.maxTCPConnections

[sgothel]

• Resolves: Handle limited open Connections due to keepalive connections (cool#9621) #9833 refining PR Handle limited open Connections due to keepalive connections #9916 and subsuming PR net::Defaults: Revert and re-apply revised code #10366 and PR Implement MaxConnection Limit (2) #10370
• Target version: master

Summary

net::Defaults.maxTCPConnections: Use WSD-config or system's maximum concurrent TCP connections

net::Defaults.maxExtConnections is set:

• use COOLWSD config net.max_ext_connections if >= COOLWSD::MinConnectedSessions
• or is disabled (zeroed) if config < 0.
• otherwise set to system-value, see Util::getMaxConcurrentTCPConnections.

Util::getMaxConcurrentTCPConnections uses Linux kernel values

• /proc/sys/net/ipv4/tcp_max_orphans
  See https://www.kernel.org/doc/html/latest/networking/ip-sysctl.html
• /proc/sys/net/nf_conntrack_max
  See https://www.kernel.org/doc/html/latest/networking/nf_conntrack-sysctl.html
• or returns zero if undefined

The Linux kernel values are memory bound, approximately 4 concurrent TCP connections per MB system memory are provided,
e.g. {4096M -> 16384}, {16384M -> 65536}, {65407M -> 262144}, ...

COOLWSD::MinConnectedSessions is a static constexpr with value 3, symbolized for clarity.

Checklist

• I have run make prettier-write and formatted the code.
• All commits have Change-Id
• I have run tests with make check
• I have issued make run and manually verified that everything looks okay
• Documentation (manuals or wiki) has been updated or is not required

[sgothel]

desktop cypress calc/searchbar_spec.js post rebase on a2f3536, while PR #10370 passed. Hence it got better .. but not fully resolved yet.

[caolanm]

I feel readDecimal and dataToDecimal are a bit ornate if we compare to say getTotalSystemMemoryKb or getFromCGroup which just open their /proc file, read a line from it and atoll on that

[sgothel]

I feel readDecimal and dataToDecimal are a bit ornate if we compare to say getTotalSystemMemoryKb or getFromCGroup which just open their /proc file, read a line from it and atoll on that

looked at them ofc and thought about adding this func .. esp due to late reviews :)
problem, the used atoll (edit: existing code) is not covering all error cases and I wanted a one liner to fetch the value
to have the getTechValue func clean of these things - encapsulation.
If its a blocker, I will revise.

[sgothel]

added a tiny fix on calling readDecimal (EOS included), refined its API doc .. kicking CI again (ole cypress failure)

[mmeeks]

Some good things here that can go in - but a number that need feedback. I -really- don't want review getting lost and ignored in monster review threads of giant collections of commits. It is good to split them up - lets also push them in small batches to different PRs.

checkRemoval -> fine.

Simplify UnitTimeoutBase -> no idea - a huge change with no clear rationale.

Add 'UT' -> the 'UT' is not needed - lets not make up lots of new acronyms in commit messages it is implicit in UnitTestFoo - otherwise good to see a new test -> fine.

is "Remove WebSocketHandler's WS Ping timeout functionality" a nearly straight revert - if so good; lets include the word 'Revert' in the commit message - otherwise fine.

Like the Remove redundant 'Socket::isClosed()' - but I would prefer us to remove 'isOpen' to minimize the end-to-end diff across the code. We don't want non-functional renames of methods isClosed -> isOpen is not a win.

"Add safe ..." don't like creating big, redundant helpers like this. We already have similar code, using other helpers that does this - git grep for & re-use the same code there.

why tcp_max_orphans ?

Thanks!

[sgothel]

Simplify UnitTimeoutBase -> no idea - a huge change with no clear rationale.

Not huge IMHO, but simplifying the unit test more to its point, which pattern is also more reusable for other users.

Add 'UT' -> the 'UT' is not needed - lets not make up lots of new acronyms in commit messages it is implicit in UnitTestFoo - otherwise good to see a new test -> fine.

Fun fact, I read this abbreviation somewhere else .. and picked it up. OK.

Like the Remove redundant 'Socket::isClosed()' - but I would prefer us to remove 'isOpen' to minimize the end-to-end diff across the code. We don't want non-functional renames of methods isClosed -> isOpen is not a win.

I counted usage of both, and isOpen was used more often without inversion
while isClosed required inversion - so this is more straight, a win.

"Add safe ..." don't like creating big, redundant helpers like this. We already have similar code, using other helpers that does this - git grep for & re-use the same code there.

std::stroll used here covers all error cases, while existing code does not (using atoll).
Initial motivation to factor it out of the actual evaluation function was to not
have it littered with ordinary and distracting I/O and conversion details.

(other items were either accepted or discussed in upcoming PRs)

[mmeeks]

When feedback is incorporated systematically please poke me to review again. Thanks.

[sgothel]

When feedback is incorporated systematically please poke me to review again. Thanks.

Done.

Remark: The requested removal of Socket::isOpen() instead of Socket::isClose() increased the diff as laid out earlier. Regardless, I hope it fits requirements as I also changed the field name to have the accessor match 1:1 w/o inverted logic.

Other changes applied as requested as well.

[sgothel]

Cleaned up branch rebased tree, i.e. PR #10375 -> #10455 -> #10456

(also refined getMaxConcurrentTCPConnections less nonsense analytics scaring comparison and borderline unsigned long long > numeric_limits<size_t>::max() check)

[mmeeks]

Lets get the first two commits in- but not the third - which needs re-working.

[mmeeks]

Some more work to do - as well as splitting from the underlying un-necessary extra commit.

[mmeeks]

Minor changes but lets get them done first.

[sgothel]

force push: rebase to master only

[sgothel]

Addressed comments, added config value etc.

[Ashod]

Thanks, @sgothel. Left some comments/notes.

[sgothel]

push:

• Using Util::u64FromString
• Refined comment hinting on tri-state of WSD config net.max_ext_connections as described in net::DefaultValues::maxExtConnections

[mmeeks]

@Ashod - can you do some thinking on whether we want this, and get it in if so ? =)

[Ashod]

@Ashod - can you do some thinking on whether we want this, and get it in if so ? =)

Yes, will do.