Skip to content

Add connection backpressure and timeouts#72

Open
gjcairo wants to merge 14 commits intomainfrom
limit-connections
Open

Add connection backpressure and timeouts#72
gjcairo wants to merge 14 commits intomainfrom
limit-connections

Conversation

@gjcairo
Copy link
Copy Markdown
Collaborator

@gjcairo gjcairo commented Mar 30, 2026

This PR adds connection-level backpressure and timeout support to the server.

A new optional maxConnections field on NIOHTTPServerConfiguration lets users cap the number of concurrent connections: when the limit is reached, the server stops accepting new connections by gating NIO read events on the server channel until existing connections close.

Alongside this, a new ConnectionTimeouts configuration provides idle, read-header, and-read body timeouts to evict slow or misbehaving connections, to mitigate attacks against the connection limit. Timeouts are enabled by default with reasonable values (60s/30s/60s respectively); the connection limit is opt-in.

Both features work across HTTP/1.1 and HTTP/2, with timeout handlers split appropriately between connection and stream levels for HTTP/2.

@gjcairo gjcairo added the ⚠️ semver/major Breaks existing public API. label Mar 30, 2026
@gjcairo gjcairo marked this pull request as ready for review March 30, 2026 14:36
@gjcairo gjcairo requested a review from aryan-25 March 30, 2026 14:36
///
/// ## Configuration keys:
/// - `idle` (int, optional, default: 60): Maximum time in seconds a connection can remain idle.
/// Set to `null` to disable.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"Set to null to disable" isn't true; each of these options will get nil-coalesced to the default value if the key isn't specified in the config.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I changed the docs but also removed the coalescing from the SwiftConfiguration initialiser - I think it's clearer that if you set things to nil then you don't want anything.

Comment thread Sources/NIOHTTPServer/NIOHTTPServer.swift Outdated
func addTimeoutHandlers(to channel: any Channel) throws {
let timeouts = self.configuration.connectionTimeouts

if let idle = timeouts.idle {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like these three if blocks are identical to the method bodies below. We can probably call into those methods here.

if let idle = self.configuration.connectionTimeouts.idle {
let idleTimeAmount = TimeAmount(idle)
try channel.pipeline.syncOperations.addHandler(
IdleStateHandler(readTimeout: idleTimeAmount, writeTimeout: idleTimeAmount)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we allow both the read and write idle timeouts to be configurable?

try await NIOHTTPServerTests.withServer(
server: server,
serverHandler: HTTPServerClosureRequestHandler { _, _, reader, responseSender in
_ = try await reader.consumeAndConclude { bodyReader in
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

)
try await outbound.write(.end(nil))

var iter = inbound.makeAsyncIterator()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

}

func read(context: ChannelHandlerContext) {
if self.activeConnections <= self.maxConnections {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this channel handler guaranteed to only prevent the call to read() once?

Because channel.read() is only called once upon close (line 48).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I might be misunderstanding your point, but the point of this handler is not to prevent the call to read just once - it's to make sure we're always below the set limit. That might mean calling read multiple times whenever we fall below the threshold, and withhold forwards of reads when we're above it.

let loopBoundContext = NIOLoopBound(context, eventLoop: context.eventLoop)
let eventLoop = context.eventLoop
childChannel.closeFuture.whenComplete { _ in
eventLoop.execute {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do we need to hop here? we already should be on the correct EL.

let context = loopBoundContext.value
self.activeConnections -= 1
if self.activeConnections <= self.maxConnections {
context.read()
Copy link
Copy Markdown
Member

@fabianfett fabianfett Apr 1, 2026

Choose a reason for hiding this comment

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

I don't think we should read here automatically. I think we should only read here, if we have seen a read call before that we didn't immediately forward. Other channels in the pipeline might want to stop backpressure for their own reasons. This auto read call assumes we are the only channel in the pipeline.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point, will make this change

///
/// The timeout starts when the channel becomes active and is cancelled when
/// a `.head` part is received.
final class ReadHeaderTimeoutHandler: ChannelInboundHandler, RemovableChannelHandler {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While it is totally fair to implement this in additional ChannelHanders, I strongly advise against it for performance reasons. We should really try to reduce pipeline overhead and pipeline modifications.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've consolidated these into two handlers instead. Is that what you had in mind?

@gjcairo gjcairo requested review from aryan-25 and fabianfett April 14, 2026 15:52
///
/// ## Configuration keys:
/// - `idle` (int, optional, default: nil): Maximum time in seconds a connection can remain idle.
/// - `readHeader` (int, optional, default: nil): Maximum time in seconds to receive request headers.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

after the connection/stream was open I assume. Can we please add that?

/// ## Configuration keys:
/// - `idle` (int, optional, default: nil): Maximum time in seconds a connection can remain idle.
/// - `readHeader` (int, optional, default: nil): Maximum time in seconds to receive request headers.
/// - `readBody` (int, optional, default: nil): Maximum time in seconds to receive the request body.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the full body? or is this between body part? please make this more explicit.

Comment on lines +247 to +249
/// Maximum time allowed to receive the complete request body
/// after headers have been received. `nil` means no timeout.
public var readBody: Duration?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this use-full? this means that this has to account for the longest possible request. I think something like the time between body parts is more appropriate?

public struct ConnectionTimeouts: Sendable {
/// Maximum time a connection can remain idle (no data read or written)
/// before being closed. `nil` means no idle timeout.
public var idle: Duration?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please call out, that this is only used after the first request. before the first request, it is readHeader.

Comment on lines 310 to +314
supportedHTTPVersions: Set<HTTPVersion>,
transportSecurity: TransportSecurity,
backpressureStrategy: BackPressureStrategy = .defaults
backpressureStrategy: BackPressureStrategy = .defaults,
maxConnections: Int? = nil,
connectionTimeouts: ConnectionTimeouts = .defaults
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are we sure we want to have this initializer? it feels like we would need to extend that, whenever a new property is added? I think having the init with just first three is more appropriate. All others can be changed via properties, if needed.

@@ -24,6 +25,9 @@ enum NIOHTTPServerConfigurationError: Error, CustomStringConvertible {

case .incompatibleTransportSecurity:
"Invalid configuration: only HTTP/1.1 can be served over plaintext. `transportSecurity` must be set to (m)TLS for serving HTTP/2."
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just as a drive by notice: I think we should also support, h/2 over plaintext, shouldn't we?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we're tracking that work.

}

@available(macOS 26.2, iOS 26.2, watchOS 26.2, tvOS 26.2, visionOS 26.2, *)
extension Channel {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think this should be extension on the syncOperation and not the channel. This would make reading the callsite much easier.

channel.eventLoop.makeCompletedFuture {
try channel.pipeline.syncOperations.addHandler(HTTP1ToHTTPServerCodec(secure: true))

try channel.addTimeoutHandlers(self.configuration.connectionTimeouts)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the call should be:

Suggested change
try channel.addTimeoutHandlers(self.configuration.connectionTimeouts)
try channel.pipeline.syncOperations.addTimeoutHandlers(self.configuration.connectionTimeouts)

/// - When `.end` is received, the body timeout is cancelled.
///
/// If either timeout fires, the connection is closed.
final class RequestTimeoutHandler: ChannelInboundHandler, RemovableChannelHandler {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we 1000% should combine those two handlers into just one. We should try to reduce the total number of ChannelHandlers as much as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ semver/major Breaks existing public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants