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.

Sign up for GitHub

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

Jump to bottom

Revise Handle Limited Open Connections #10355

Closed
wants to merge 13 commits into from
Closed

Revise Handle Limited Open Connections #10355

wants to merge 13 commits into from

Conversation

sgothel
Copy link

@sgothel sgothel commented Oct 28, 2024 •
edited
Loading

Summary

This track follows up on PR #10334 and may include PR for issue #10337

Functional

Style

  • Removes immutable member variables on frequently created objects (copies of global settings)
    • In particular net::Defaults related
  • Fixes (some) naming conventions breakage
  • Removes redundant boolean SocketPoll::_limitedConnections, use query method instead
  • Removes SocketPoll's itemsErased accounting while erasing
  • Removes redundant Socket::isClosed()

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 sgothel added the 24.04 label Oct 28, 2024
@sgothel sgothel self-assigned this Oct 28, 2024
This was referenced Oct 28, 2024
Merged
Closed
@sgothel
Copy link
Author

sgothel commented Oct 28, 2024

@mmeeks wrote in #10337 (comment)

It is -very- unclear to me why we want to ever close websockets; and I'm none the wiser having read this.

A websocket once setup and authenticated is used for hard-lifecycle management of an editing session.

Close it frivolously and really-bad-things can happen - in extremis data-loss if permissions changed underneath us and we can no longer ask the user what to do about a failed save. I see no reason to have added this closing of websocket at all FWIW - and I'd prefer it backed out ASAP until there is a clear understanding and concept around what we're trying to achieve here.

Will remove this functional block

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 29, 2024
View reviewed changes
common/FileUtil.hpp Dismissed Show resolved Hide resolved
@sgothel sgothel force-pushed the private/sgothel/socketcode_repair branch from ff7f556 to 32b688b Compare October 29, 2024 08:44
@sgothel
Copy link
Author

sgothel commented Oct 29, 2024

All checks have passed .. will rebase to master now .. (no merge yet, duh!)

@sgothel sgothel force-pushed the private/sgothel/socketcode_repair branch from 32b688b to 92f7113 Compare October 29, 2024 09:38
mmeeks
mmeeks reviewed Oct 29, 2024
View reviewed changes
net/Socket.cpp Show resolved Hide resolved
@sgothel
Copy link
Author

sgothel commented Oct 29, 2024

A weirdo merge conflict occurred via github here, which wasn't existing locally (rebased to master).
"Fun" ..

@mmeeks
Copy link
Contributor

mmeeks commented Oct 29, 2024

So - there is once again a huge amount of change here - a ton of it is not related to reverting back to the state before we started making huge amounts of change. Again - I want to restore us to a known-safe state tested on millions of users - and then we take very, very small steps, incrementally, without huge and questionable cosmetic changes on top to improve the code.

Perhaps it is nice to do some stream logging operator improvement - perhaps that makes people feel good - but to do that mixed with functional change, and/or to do it at all now - when the priority is getting us back to a known good state is highly unhelpful.

Perhaps it would be better to have a more simple approach. Revert everything that went in recently - and then go over it again - breaking it down properly, thinking through the design carefully, and so on. At this point I'm basically not interested in any new features - connection limits, trottling, closing websockets, etc. - I want us back where we were.

Sven Göthel added 13 commits October 29, 2024 11:29
StreamSocket::checkRemoval: Drop virtual Socket:checkRemoval, Rename …
7b39f81 
…{idle/poll} -> inactivity and use net::Defaults::InactivityTimeout (3600s)

Drop virtual Socket:checkRemoval
- checkRemoval is only called from `StreamSocket::handlePoll`,
  see commit d40887b

Rename {idle/poll} -> inactivity and use net::Defaults::InactivityTimeout (3600s)
- Clarify semantics w/ proper names
- Use a commonly used keep-alive timeout value of 1 hour

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Cleanup UTs: Simplify UnitTimeoutBase and update net::Defaults annota…
cb5f78b 
…tions in UnitTimeout{Connections, None, WSPing}

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Add UT UnitTimeoutInactive unit-timeout_inactive, covering inactivi…
b18cff1 
…ty TO of `StreamSocket::checkRemoval`

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Use net::Defaults as a global, drop its SocketPollTimeout, HTTPTimeou…
17b54b4 
…t and revert to original. Drop UnitWSD::configNet().

Partially reverts commit 9887ed1

- Use `net::Defaults` as a global, removing the singleton pattern
- Drop `net::Defaults`'s `SocketPollTimeout`, `HTTPTimeout` fields and revert to original
  - Notable: `StreamSocket::parseHeader` uses `SocketPoll::DefaultPollTimeoutMicroS`
    for `delayMax` error-handling again (64s) instead of `_httpTimeout` (30s).
- Drop `UnitWSD::configNet()` and just use `UnitWSD::configure()` for timeout UTs.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
WS Ping: Use common stream-out methods
f2fd851 
Use common stream-out method
- net::Defaults
- WebSocketHandler
  - add WS Ping details to dumpState()

No change of semantics.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Remove WebSocketHandler's WS Ping timeout functionality
cec726d 
WS Ping timeout functionality was added in commit 994f8f9
and is removed with this patch as not deemed desirable anymore.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Remove redundant Socket::isClosed()
a5a38b6 
Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
SocketPoll::poll: Remove accounting and logging of itemsErased
381c535 
Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
SocketPoll: Rename {S->_s}tatsMutex, {S->_s}tatsConnectionCount, {S->…
024cd26 
…s}tatsConnectionMod, {G->g}etStatsConnectionCount

Note that `_statsConnectionCount` requires a lock for writing
inside `statsConnectionMod` due to its global nature for many
outside facing SocketPoll instances.

Read only `getStatsConnectionCount` simply utilizes an CST atomic
for lock-free operations. Its usage within `SocketPoll::poll`
is well working as it would only use old data to block new connections
and is freuqntly called.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
SocketPoll: Replace _limitedConnections w/ isConnectionLimited qu…
7d6d5d8 
…ery on `_connectionLimit > 0`

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
SocketPoll::setLimiter: Adjust for max-value corner case
19bf60d 
Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Add safe Util::dataToDecimal<T> and FileUtil::readDecimal to read…
e17d816 
… single decimals from files like `/proc/sys/net/ipv4/tcp_max_orphans`

Implementation is value- and thread-safe, utilizing `std::strtoll`.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
Socket MaxTCPConnections Limiter (New Connections): Use system's maxi…
5577efe 
…mum concurrent TCP connections, disable if undefined

Used system value on a Linux kernel are
- /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

Previous confusion about 'max connections' semantics has been resolved,
as this `net::Default::maxTCPConnections` is unrelated to session count
but actual outfacing TCP connections only.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Iad74f253bdac5636757b130b299b5deacda658db
@sgothel sgothel force-pushed the private/sgothel/socketcode_repair branch from 218abb3 to 5577efe Compare October 29, 2024 10:30
@sgothel
Copy link
Author

sgothel commented Oct 29, 2024 •
edited
Loading

rebased again, mmeeks branch (w/ some same commits) got merged, hence earlier confusion (collision).

@sgothel
Copy link
Author

sgothel commented Oct 29, 2024

Will be revised to fully revert 2 commits which have been partially reverted and reapply changes on top of vanilla.

@sgothel sgothel closed this Oct 29, 2024
This was referenced Oct 29, 2024
Merged
Merged
Merged
@sgothel sgothel deleted the private/sgothel/socketcode_repair branch November 1, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmeeks mmeeks mmeeks left review comments

@Ashod Ashod Awaiting requested review from Ashod

@caolanm caolanm Awaiting requested review from caolanm

Assignees

@sgothel sgothel

Labels
24.04
Projects
Collabora Online
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle limited open Connections due to keepalive connections (cool#9621)
2 participants
@sgothel @mmeeks