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

fix: enhance connection rate limit #196

Merged
merged 21 commits into from
Nov 13, 2024

Conversation

zmstone
Copy link
Member

@zmstone zmstone commented Nov 9, 2024

Fixes: emqx/emqx#14145

Prior to this change, if connection rate is limited, the acceptors will enter suspending state and stop accepting the sockets leaving the sockets in the system backlog.

If the acceptor backlog (default=1024) is filled up, for long enough time to cause the majority of the clients to have closed socket from their end and try to reconnect aggressively, the acceptor may never be able to get a normal socket again.

The fix is: in suspending state, accept the sockets and immediately close them to free up the backlog.
The close triggers TCP-RST to cut the TCP graceful close overheads.

@zmstone zmstone changed the title feat: enhance connection rate limit fix: enhance connection rate limit Nov 9, 2024
@zmstone zmstone force-pushed the 241109-accept-but-close-when-conn-rate-limit-reached branch from e0fe870 to d3e15b9 Compare November 9, 2024 11:16
@zmstone zmstone force-pushed the 241109-accept-but-close-when-conn-rate-limit-reached branch 3 times, most recently from 2de8851 to 1a86898 Compare November 11, 2024 17:30
Prior to this change, if connection rate is limited, the acceptors
will enter suspending state and stop accepting the sockets
leaving the sockets in the system backlog.

If the acceptor backlog (default=1024) is filled up, for long enough
time to cause the majority of the clients to have closed socket from
their end and try to reconnect aggressively, the acceptor may never
be able to get a normal socket again.

The fix is: in suspending state, accept the sockets and immediately
cose them to free up the backlog.
The close triggers TCP-RST to cut the TCP graceful close overheads.
@zmstone zmstone force-pushed the 241109-accept-but-close-when-conn-rate-limit-reached branch from 85445bd to 4f1a472 Compare November 11, 2024 18:09
@zmstone zmstone force-pushed the 241109-accept-but-close-when-conn-rate-limit-reached branch 2 times, most recently from 910c448 to 0031552 Compare November 11, 2024 18:34
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
case eval_tune_socket_fun(TuneFun, Sock) of
{ok, Sock} ->
case esockd_connection_sup:start_connection(ConnSup, Sock, UpgradeFuns) of
{ok, NewSock} ->
Copy link
Contributor

Choose a reason for hiding this comment

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

why the socket is changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

the socket does not change most of the time, but the API is designed to allow it to change.

{ok, _Pid} ->
%% Inc accepted stats.
inc_stats(State, accepted),
Copy link
Contributor

Choose a reason for hiding this comment

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

why move to here? I think the old code looks fine. the socket is indeed accepted isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

there was previously no clear definition what accepted means.
and there was no counters for error cases.

now it means: accepted from the listener backlog, and successfully started an owner process.
and this counter is disjoint from error cases.

src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_server.erl Outdated Show resolved Hide resolved
test/esockd_acceptor_SUITE.erl Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor_sup.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
@zmstone zmstone force-pushed the 241109-accept-but-close-when-conn-rate-limit-reached branch from a2d6096 to 73f9fab Compare November 12, 2024 10:14
@zmstone zmstone force-pushed the 241109-accept-but-close-when-conn-rate-limit-reached branch from 73f9fab to 10475ca Compare November 12, 2024 10:17
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
src/esockd_acceptor.erl Outdated Show resolved Hide resolved
@zmstone zmstone merged commit c784179 into master Nov 13, 2024
3 checks passed
@zmstone zmstone deleted the 241109-accept-but-close-when-conn-rate-limit-reached branch November 13, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EMQX Max Conn Rate Limiter does not work. Hangs the entire listener with 100% CPU utilization
3 participants