-
Notifications
You must be signed in to change notification settings - Fork 423
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
Add max connections to netty configuration #3053
Conversation
server/netty-server/src/main/scala/sttp/tapir/server/netty/NettyConfig.scala
Outdated
Show resolved
Hide resolved
|
||
import java.util.concurrent.atomic.AtomicInteger | ||
|
||
@Sharable case class NettyConnectionCounter(maxConnections: Int) extends ChannelInboundHandlerAdapter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, according documentation with this annotation in Netty class can be safely shared among multiple connections and it prevent race condition.
@@ -22,10 +22,13 @@ object NettyBootstrap { | |||
.childHandler(new ChannelInitializer[Channel] { | |||
override def initChannel(ch: Channel): Unit = { | |||
val nettyConfigBuilder = nettyConfig.initPipeline(nettyConfig) | |||
val connectionCounter = NettyConnectionCounter(nettyConfig.maxConnections) | |||
val pipelineWithConnectionCounter = ch.pipeline().addFirst(connectionCounter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we modify the pipeline after calling initPipeline
to reliably add this as the first handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so initPipeline
return a function which is always call with pipelineWithConnectionCounter
parameter. This parameter ensures that connectionCounter
is added as the first handler, so i think that code do what you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then we pass it to nettyConfigBuilder
which might add other handlers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but i add connectionCounter
by .addFirst
method, which ensure that connectionCounter will be execute before any other handlers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But can't code in nettyConfigBuilder
add another handler using .addFirst
and effectively install an even earlier handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in that case it is possible, so do you have any ideas how to prevent this situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - call .addFirst after calling nettyConfigBuilder
- won't that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nettyConfigBuilder
returns Unit, I need add .addFirst
to the parameter in nettyConfigBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a side-effecting operation, so you can simply grab the pipeline from ch.pipelie()
and modify it
|
||
override def channelActive(ctx: ChannelHandlerContext): Unit = { | ||
val counter = connections.incrementAndGet | ||
if (counter <= maxConnections) super.channelActive(ctx) else ctx.close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Before closing, let's log a logger.warning(s"Max connections exceeded: $maxConnections")
You can create a logger with
import io.netty.util.internal.logging.InternalLoggerFactory
// ...
private lazy val logger = InternalLoggerFactory.getInstance(getClass)
Afterwards, please test this locally with a low number, like 2. You can have an endpoint which sleeps for a few seconds, then call it many times, in order to generate exceeding max connections. Please verify if this counter works as expected in such a case and if the warning is indeed logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tested this together with Adrian using Apache Benchmark (terminal ab
tool for sending concurrent requests). The counter seems to be growing and shrinking according to expectations, and exceeding it closes the channel.
One more improvement we may consider is returning an HTTP 503 or 509 (for some reason 509 is not present in Netty constants) instead of breaking the connection, but this may be problematic, as some of my attempts are showing.
Looks good, thanks! |
The next point from list.
I made it on the base: https://stackoverflow.com/questions/46849964/how-to-set-maximum-concurrent-connections-with-netty