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][flaky-test]ProxyConnectionThrottlingTest.testInboundConnection #17883

Merged

Conversation

poorbarcode
Copy link
Contributor

Fixes:

Motivation

this test is executed as follows:

  1. set max connection limited 4
  2. create two clients and two producers, this will create 4 connections
  3. try to create another client and producer, this will fail cause of reaches the max connections count limit

But the execution logic of step-3 is like this:

  1. increment DefaultConnectionController.totalConnectionNum
  2. if the state State.REACH_MAX_CONNECTION_PER_IP founded, do close connection which will decrement DefaultConnectionController.totalConnectionNum

Then the problem is that the connection closes was executed asynchronously(see code below), if the main thread executes fast enough, the assert Assert.assertEquals(ConnectionController.DefaultConnectionController.getTotalConnectionNum(), 4) will fail, because the last connection has not close finish.

public void channelRegistered(ChannelHandlerContext ctx) throws Exception {
super.channelRegistered(ctx);
ProxyService.ACTIVE_CONNECTIONS.inc();
SocketAddress rmAddress = ctx.channel().remoteAddress();
ConnectionController.State state = connectionController.increaseConnection(rmAddress);
if (!state.equals(ConnectionController.State.OK)) {
ctx.writeAndFlush(Commands.newError(-1, ServerError.NotAllowedError,
state.equals(ConnectionController.State.REACH_MAX_CONNECTION)
? "Reached the maximum number of connections"
: "Reached the maximum number of connections on address" + rmAddress))
.addListener(result -> ctx.close());
ProxyService.REJECTED_CONNECTIONS.inc();
}

Modifications

Change the assertion logic to executed using Awaitility

Documentation

  • doc-not-needed

Matching PR in forked repository

PR in forked repository:

@lhotari lhotari closed this Sep 29, 2022
@lhotari lhotari reopened this Sep 29, 2022
Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

Thanks a lot!!

@lhotari lhotari merged commit 3de690d into apache:master Sep 29, 2022
@poorbarcode poorbarcode deleted the flaky/ProxyConnectionThrottlingTest branch September 29, 2022 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants