Total connections should stay positive #360
Conversation
...nents/server/src/main/java/com/hotels/styx/server/netty/handlers/ConnectionCountHandler.java
Outdated
Show resolved
Hide resolved
| @@ -149,6 +150,7 @@ public void configure(Channel channel, HttpHandler httpPipeline) { | |||
| .addLast("connection-throttler", excessConnectionRejector) | |||
| .addLast("idle-handler", new IdleStateHandler(serverConfig.requestTimeoutMillis(), 0, serverConfig.keepAliveTimeoutMillis(), MILLISECONDS)) | |||
| .addLast("channel-stats", channelStatsHandler) | |||
| .addLast("channel-connection-stats", new ConnectionCountHandler(metrics)) | |||
There was a problem hiding this comment.
Remember the root cause for this bug: An ExcessConnectionRejector causes a channelInactive event being sent without preceding a channelActive. Therefore, we should fix the ExcessConnectionRejector and ensure channelActive is always sent before channelInactive.
For example, I sketched something like this:
public class ExcessConnectionRejector extends ChannelInboundHandlerAdapter {
private static final Logger LOGGER = getLogger(ExcessConnectionRejector.class);
private final ChannelGroup channelGroup;
private final int maxConnectionsCount;
private boolean channelActivated;
public ExcessConnectionRejector(ChannelGroup channelGroup, int maxConnectionsCount) {
checkArgument(maxConnectionsCount > 0);
this.channelGroup = requireNonNull(channelGroup);
this.maxConnectionsCount = maxConnectionsCount;
}
@Override
public void channelRegistered(ChannelHandlerContext ctx) throws Exception {
if (channelGroup.size() >= maxConnectionsCount) {
LOGGER.warn("Max allowed connection to server exceeded: current={} configured={}", channelGroup.size(), maxConnectionsCount);
ctx.close();
return;
}
channelGroup.add(ctx.channel());
super.channelRegistered(ctx);
}
@Override
public void channelActive(ChannelHandlerContext ctx) throws Exception {
LOGGER.info("channel active");
channelActivated = true;
super.channelActive(ctx);
}
@Override
public void channelInactive(ChannelHandlerContext ctx) throws Exception {
LOGGER.info("channel inactive");
if (channelActivated) {
super.channelInactive(ctx);
}
}
}
You would of course make ExcessConnectionRejector non-shared. You can also move the event order enforcing logic out into a separate Netty handler if you so wish.
With this approach you don't need to widen the scope of this PR onto other non-relevant classes.
There was a problem hiding this comment.
This is not entirely true - the channelActivated field needs to be in a non-shared handler, but the channel count needs to be shared. However, it is true that the implementation can be simplified and I will do so.
(Mikko is no longer reviewing this PR, but I am leaving this comment so that other reviewers understand that I have taken the words into account.)
| if (eventProcessor != null) { | ||
| eventProcessor.submit(new ChannelInactiveEvent()); | ||
| } | ||
|
|
There was a problem hiding this comment.
These changes can be backed off after fixing the Excess Connection Rejector which will enforce the correct event ordering.
|
|
||
| super.channelInactive(ctx); | ||
| } | ||
|
|
There was a problem hiding this comment.
This class can also be left alone.
|
I have made some changes, but it appears to intermittently fail when running the |
… two errors will be thrown - that or BadHttpResponseException)
|
Regarding my earlier comment, it looks like the test client will sometimes see the failure to connect as |
| @@ -37,11 +37,11 @@ | |||
| public class ChannelStatisticsHandler extends ChannelDuplexHandler { | |||
| private static final Logger LOGGER = LoggerFactory.getLogger(ChannelStatisticsHandler.class); | |||
|
|
|||
| public static final String PREFIX = "connections"; | |||
| static final String PREFIX = "connections"; | |||
There was a problem hiding this comment.
Why are some of these static fields package-private and others private? I don't see all of them used in tests or elsewhere outside the class... For the ones that are used in tests (e.g., RECEIVED_BYTES), do we usually annotate them with VisibleForTesting ?
There was a problem hiding this comment.
This was done using the suggestions from my IDE. I will see if anything can be done to clean it up. Usually for our tests, we don't reference constants, but just use the literal values.
There was a problem hiding this comment.
The issue that our excessConnectionRejector sends a channelInactive without a corresponding channelActive event is not fixed at all, is it? I dont'see many differences with how Netty deals with it in similar code but we might want to add parts of what they are doing (e.g., removing the handler from the pipeline). I don't know if we have looked into what the state of the network is in 'channelRegistered' , it might make sense to move the code to channelActive...
https://github.com/netty/netty/blob/22b2c4c3b8be75ad3ea97f115d1bc56fa46069d2/handler/src/main/java/io/netty/handler/ipfilter/AbstractRemoteAddressFilter.java
A fix for #351