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

Implement MaxConnection Limit (2) #10370

Merged
merged 2 commits into from
Nov 1, 2024
Merged

Conversation

sgothel
Copy link

@sgothel sgothel commented Oct 30, 2024

This PR is on top of #10366 !

Summary

Reimplementation of commit 80246f7 (post revert).

Adding external TCP connection limit to server-side TCP IPv4/IPv6 Sockets

  • Counted at StreamSocket ctor
    • Only limits TCP connections for server-side IPv4 or IPv6 TCP connections.
  • Rejected at ServerSocker::handlePoll
    • If exceeding net::Defaults.maxExtConnections, socket object and hence connection is dropped

TODO

  • revise net::Defaults.maxTCPConnections to match actual system settings
    • earmarked for subsequent PR

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 30, 2024
@sgothel sgothel self-assigned this Oct 30, 2024
@sgothel sgothel force-pushed the private/sgothel/socketcode_maxconn branch from 7031ce0 to 80b7531 Compare October 30, 2024 09:35
@sgothel
Copy link
Author

sgothel commented Oct 30, 2024

Forced pushed (no1): ServerSocket::accept usage of socket didn't work w/ mobile build - reverted to _socket again, which also minimized the delta a little.

@sgothel
Copy link
Author

sgothel commented Oct 30, 2024

Forced pushed (no2): Moved limit-check/registration to StreamSocket::create()

@sgothel sgothel force-pushed the private/sgothel/socketcode_maxconn branch 2 times, most recently from 826374c to 013278f Compare October 30, 2024 15:49
Copy link
Contributor

@mmeeks mmeeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting better; lets simplify this further; I believe we should be able to do this in around 10 lines.

net/Socket.cpp Outdated Show resolved Hide resolved
net/Socket.cpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
net/Socket.hpp Outdated Show resolved Hide resolved
@mmeeks
Copy link
Contributor

mmeeks commented Oct 30, 2024

Would be nice to get the base commits in and re-base this of course too =) I queued another build.

@sgothel sgothel force-pushed the private/sgothel/socketcode_maxconn branch 2 times, most recently from 136e95a to 631db5f Compare October 31, 2024 14:49
@mmeeks mmeeks enabled auto-merge (rebase) October 31, 2024 15:46
@mmeeks
Copy link
Contributor

mmeeks commented Oct 31, 2024

Great - hopefully we can re-base as-is on whatever fixes are going in for CI

@sgothel sgothel force-pushed the private/sgothel/socketcode_maxconn branch from 631db5f to 5743363 Compare October 31, 2024 16:29
@sgothel
Copy link
Author

sgothel commented Oct 31, 2024

Small fix force pushed, detected with subsumed PR ... (missed this config case)

            if( extConnCount <= net::Defaults.maxExtConnections )
            if( 0 == net::Defaults.maxExtConnections || extConnCount <= net::Defaults.maxExtConnections )

@caolanm caolanm force-pushed the private/sgothel/socketcode_maxconn branch from 5743363 to 4a4bff4 Compare November 1, 2024 09:31
@caolanm caolanm disabled auto-merge November 1, 2024 09:33
Sven Göthel added 2 commits November 1, 2024 11:32
This reverts commit 80246f7.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Ib9f0ac17c05ffe65c2370490f68f581fa76730e7
Reimplementation of commit 80246f7 (post revert).

Adding external TCP connection limit to server-side TCP IPv4/IPv6 Sockets
- Counted at StreamSocket ctor
  - Only limits TCP connections for server-side IPv4 or IPv6 TCP connections.
- Rejected at ServerSocker::handlePoll
  - If exceeding net::Defaults.maxExtConnections, socket object and hence connection is dropped

net::Defaults
  - Renamed maxTCPConnections -> maxExtConnections

TODO:
- revise net::Defaults.maxExtConnections to match actual system settings

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Ib9f0ac17c05ffe65c2370490f68f581fa76730e7
@sgothel sgothel force-pushed the private/sgothel/socketcode_maxconn branch from 4a4bff4 to 57f6414 Compare November 1, 2024 10:38
@caolanm caolanm enabled auto-merge (rebase) November 1, 2024 12:17
@caolanm caolanm disabled auto-merge November 1, 2024 12:25
@caolanm caolanm merged commit 5cd0c39 into master Nov 1, 2024
13 checks passed
@caolanm caolanm deleted the private/sgothel/socketcode_maxconn branch November 1, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Handle limited open Connections due to keepalive connections (cool#9621)
3 participants