Cleanup Socket/SocketPoll 'Limited open Connections' Changes

[sgothel]

• Resolves: Handle limited open Connections due to keepalive connections (cool#9621) #9833
• Target version: master

Summary

Originally contained PR #10270 fixing a regression caused by PR #9916, leading to 100% CPU utilization on idle,
due to WebSocketHandler's checkTimeout not being called and hence no ping sent.

This PR contains remaining cleanups related to PR #9916

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]

Unrelated UT error to this PR (from initial push, pre 1st rebase to master) unit-wopi-languages:

wsd-146207-146523 2024-10-18 17:07:15.561055 +0000 [ docbroker_001 ] TST  UnitWopiLanguages [onDocumentSaved] (+2419ms): Document failed to save| UnitWOPILanguages.cpp:68
wsd-146207-146523 2024-10-18 17:07:15.561069 +0000 [ docbroker_001 ] TST  UnitWopiLanguages [exitTest] (+2419ms): ERROR: FAILURE: exitTest: TestResult::Failed: Failed to save the document (Core is out-of-date or it has a regression: unocommandresult: { "commandName": ".uno:Save", "success": false }| common/Unit.cpp:561
wsd-146207-146523 2024-10-18 17:07:15.561085 +0000 [ docbroker_001 ] TST  UnitWopiLanguages [exitTest] (+2419ms): Dumping state| common/Unit.cpp:567
wsd-146207-146523 2024-10-18 17:07:15.561111 +0000 [ docbroker_001 ] TST  UnitWopiLanguages [endTest] (+2419ms): Ending test by disconnecting 2 connection(s): Failed to save the doc

Not reproduced here locally -> a new instability?

[sgothel]

cypress test usual failure writer/invalidations_spec.js

[mmeeks]

"Socket checkRemoval and handlePoll must be public" - surely they already were - there is no change in their visibility here surely. Please remove this apparently redundant commit that just shuffles code. Fixing the unexpectedly long and poorly wrapped comment on checkRemoval would be good in a follow-up. I'd really prefer to see a single clean fix for the bug at hand that we can merge separated from other stuff.

Similarly - the "WebSocketHandler Logging: Use LOG_DBG for low-frequency ping/pong logging (was LOGA_TRC for high-frequency), use common prefix "Ping:"" - seems like it pops out a lot of underlying channel debugging that should not be necessary. Our websocket & socket channel code has been working extremely well for a long time now - sure if we broke it we need to turn logging on for a bit; the prefix is fine - a debugging patch is fine - but we really shouldn't have great swathes of Pinging going into the logs unless it is enabled. Please use LOGA_DBG if you think it should be a higher level.

I will split out the fix patch I guess so we can merge something fast here.

[mmeeks]

With the split out fix merged; we have time to improve this I think as per comments.

[sgothel]

"Socket checkRemoval and handlePoll must be public" - surely they already were - there is no change in their visibility here surely. Please remove this apparently redundant commit that just shuffles code. Fixing the unexpectedly long and poorly wrapped comment on checkRemoval would be good in a follow-up. I'd really prefer to see a single clean fix for the bug at hand that we can merge separated from other stuff.

Yes both were public in the base class, yes checkRemoval was also public in StreamSocket
but I rearranged both .. the confusion.

However: StreamSocket::handlePoll was in a protected block, accepted by the compiler.
Socket::handlePoll is public, so shall be StreamSocket::handlePoll's override

Hence this is now a smaller cleanup fix.

Similarly - the "WebSocketHandler Logging: Use LOG_DBG

OK

I will split out the fix patch I guess so we can merge something fast here.

Thanks!