-
Notifications
You must be signed in to change notification settings - Fork 5
Add connection backpressure and timeouts #72
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
base: main
Are you sure you want to change the base?
Changes from all commits
2fb47cc
5e0742e
7569b3b
12ebb34
4944464
92ea289
42f3ecf
676da34
26ee0d3
4495e5b
0a312ab
6915d49
3aeb28d
0a86d08
0634fee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,6 +230,51 @@ public struct NIOHTTPServerConfiguration: Sendable { | |
| } | ||
| } | ||
|
|
||
| /// Configuration for connection timeouts. | ||
| /// | ||
| /// Timeouts are enabled by default with reasonable values to protect against | ||
| /// slow or idle connections. Individual timeouts can be disabled by setting | ||
| /// them to `nil`. | ||
| public struct ConnectionTimeouts: Sendable { | ||
| /// Maximum time an established connection can remain idle (no data read or written) | ||
| /// before being closed. `nil` means no idle timeout. | ||
| public var idle: Duration? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This timeout runs from connection establishment and is reset every time something is read or written, it's independent of the readHeader timeout.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, so after connection establishment, there are two timers:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One is at the connection level and the other is at request level. For H1 I agree they overlap a bit, but for H2, these are pretty different things since once operates on the connection channel and the other operates at the stream channel. |
||
|
|
||
| /// Maximum time allowed to receive the complete request headers | ||
| /// after a connection is established. `nil` means no timeout. | ||
| public var readHeader: Duration? | ||
|
|
||
| /// Maximum time allowed to receive the complete request body | ||
| /// after headers have been received. `nil` means no timeout. | ||
| public var readBody: Duration? | ||
|
Comment on lines
+247
to
+249
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a fair question. Go's http package only has a single timeout from connection establishment till connection end (so, the time it takes to read everything, including headers and request body). I think having a timeout only between body parts could open the door to attacks where a client opens a bunch of connections and slowly trickles tiny bits of data just before the timeout fires to keep the connection alive. We could have both, but we already have the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in favor of the separate read and write timeouts. Because if the client doesn't accept any writes, we are also in a bad situation. My question remains, how do you set a reasonable Also the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is btw. not a connectiontimeout but more a request timeout.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you thought of doing a rate based check when reading the body. Expect a minimum number of bytes to have been received over a defined period of time. |
||
|
|
||
| /// - Parameters: | ||
| /// - idle: Maximum idle time before the connection is closed. | ||
| /// - readHeader: Maximum time to receive request headers after a connection is established. | ||
| /// - readBody: Maximum time to receive the complete request body after headers have been received. | ||
| public init( | ||
| idle: Duration? = Self.defaultIdle, | ||
| readHeader: Duration? = Self.defaultReadHeader, | ||
| readBody: Duration? = Self.defaultReadBody | ||
|
Comment on lines
+256
to
+258
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we add another timeout we have to add another initializer. Why not just add a |
||
| ) { | ||
| self.idle = idle | ||
| self.readHeader = readHeader | ||
| self.readBody = readBody | ||
| } | ||
|
|
||
| @inlinable | ||
| static var defaultIdle: Duration? { .seconds(60) } | ||
|
|
||
| @inlinable | ||
| static var defaultReadHeader: Duration? { .seconds(30) } | ||
|
|
||
| @inlinable | ||
| static var defaultReadBody: Duration? { .seconds(60) } | ||
|
|
||
| /// Default timeout values: 60s idle, 30s read header, 60s read body. | ||
| public static var defaults: Self { .init() } | ||
| } | ||
|
|
||
| /// Network binding configuration | ||
| public var bindTarget: BindTarget | ||
|
|
||
|
|
@@ -242,18 +287,31 @@ public struct NIOHTTPServerConfiguration: Sendable { | |
| /// Backpressure strategy to use in the server. | ||
| public var backpressureStrategy: BackPressureStrategy | ||
|
|
||
| /// The maximum number of concurrent connections the server will accept. | ||
| /// | ||
| /// When this limit is reached, the server stops accepting new connections | ||
| /// until existing ones close. `nil` means unlimited (the default). | ||
| public var maxConnections: Int? | ||
|
|
||
| /// Configuration for connection timeouts. | ||
| public var connectionTimeouts: ConnectionTimeouts | ||
|
|
||
| /// Create a new configuration. | ||
| /// - Parameters: | ||
| /// - bindTarget: A ``BindTarget``. | ||
| /// - supportedHTTPVersions: The HTTP protocol versions the server should support. | ||
| /// - transportSecurity: The transport security mode (plaintext, TLS, or mTLS). | ||
| /// - backpressureStrategy: A ``BackPressureStrategy``. | ||
| /// Defaults to ``BackPressureStrategy/watermark(low:high:)`` with a low watermark of 2 and a high of 10. | ||
| /// - maxConnections: The maximum number of concurrent connections. `nil` means unlimited. | ||
| /// - connectionTimeouts: The connection timeout configuration. | ||
| public init( | ||
| bindTarget: BindTarget, | ||
| supportedHTTPVersions: Set<HTTPVersion>, | ||
| transportSecurity: TransportSecurity, | ||
| backpressureStrategy: BackPressureStrategy = .defaults | ||
| backpressureStrategy: BackPressureStrategy = .defaults, | ||
| maxConnections: Int? = nil, | ||
| connectionTimeouts: ConnectionTimeouts = .defaults | ||
|
Comment on lines
310
to
+314
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| ) throws { | ||
| // If `transportSecurity`` is set to `.plaintext`, the server can only support HTTP/1.1. | ||
| // To support HTTP/2, `transportSecurity` must be set to `.tls` or `.mTLS`. | ||
|
|
@@ -267,10 +325,16 @@ public struct NIOHTTPServerConfiguration: Sendable { | |
| throw NIOHTTPServerConfigurationError.noSupportedHTTPVersionsSpecified | ||
| } | ||
|
|
||
| if let maxConnections, maxConnections <= 0 { | ||
| throw NIOHTTPServerConfigurationError.invalidMaxConnections | ||
| } | ||
|
|
||
| self.bindTarget = bindTarget | ||
| self.supportedHTTPVersions = supportedHTTPVersions | ||
| self.transportSecurity = transportSecurity | ||
| self.backpressureStrategy = backpressureStrategy | ||
| self.maxConnections = maxConnections | ||
| self.connectionTimeouts = connectionTimeouts | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| enum NIOHTTPServerConfigurationError: Error, CustomStringConvertible { | ||
| case noSupportedHTTPVersionsSpecified | ||
| case incompatibleTransportSecurity | ||
| case invalidMaxConnections | ||
|
|
||
| var description: String { | ||
| switch self { | ||
|
|
@@ -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." | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah we're tracking that work. |
||
|
|
||
| case .invalidMaxConnections: | ||
| "Invalid configuration: `maxConnections` must be greater than 0." | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift HTTP Server open source project | ||
| // | ||
| // Copyright (c) 2026 Apple Inc. and the Swift HTTP Server project authors | ||
| // Licensed under Apache License v2.0 | ||
| // | ||
| // See LICENSE.txt for license information | ||
| // See CONTRIBUTORS.txt for the list of Swift HTTP Server project authors | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import NIOCore | ||
|
|
||
| /// A channel handler installed on the server (parent) channel that limits the | ||
| /// number of concurrent connections by gating `read()` calls. | ||
| /// | ||
| /// When the number of active connections reaches `maxConnections`, this handler | ||
| /// stops forwarding `read()` events, which prevents NIO from calling `accept()` | ||
| /// on the listening socket. When a connection closes and count drops below the | ||
| /// limit, `read()` is re-triggered to resume accepting. | ||
| final class ConnectionLimitHandler: ChannelDuplexHandler { | ||
| typealias InboundIn = Channel | ||
| typealias InboundOut = Channel | ||
| typealias OutboundIn = Channel | ||
|
|
||
| private let maxConnections: Int | ||
| private var activeConnections: Int = 0 | ||
| private var pendingRead: Bool = false | ||
|
|
||
| init(maxConnections: Int) { | ||
| self.maxConnections = maxConnections | ||
| } | ||
|
|
||
| func channelRead(context: ChannelHandlerContext, data: NIOAny) { | ||
| let childChannel = self.unwrapInboundIn(data) | ||
| self.activeConnections += 1 | ||
|
|
||
| let loopBoundSelf = NIOLoopBound(self, eventLoop: context.eventLoop) | ||
| let loopBoundContext = NIOLoopBound(context, eventLoop: context.eventLoop) | ||
| let eventLoop = context.eventLoop | ||
| childChannel.closeFuture.whenComplete { _ in | ||
| eventLoop.execute { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 `self` = loopBoundSelf.value | ||
| let context = loopBoundContext.value | ||
| `self`.activeConnections -= 1 | ||
| if `self`.pendingRead && `self`.activeConnections <= `self`.maxConnections { | ||
| `self`.pendingRead = false | ||
| context.read() | ||
|
fabianfett marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| context.fireChannelRead(data) | ||
| } | ||
|
|
||
| func read(context: ChannelHandlerContext) { | ||
| if self.activeConnections <= self.maxConnections { | ||
|
aryan-25 marked this conversation as resolved.
|
||
| context.read() | ||
| } else { | ||
| self.pendingRead = true | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -29,8 +29,8 @@ let serverConfiguration = try NIOHTTPServerConfiguration(config: config) | |||||
|
|
||||||
| ### Configuration key reference | ||||||
|
|
||||||
| ``NIOHTTPServerConfiguration`` is comprised of four components. Provide the configuration for each component under its | ||||||
| respective key prefix. | ||||||
| ``NIOHTTPServerConfiguration`` is comprised of several components. Provide the configuration for each component under | ||||||
| its respective key prefix. | ||||||
|
|
||||||
| > Important: HTTP/2 cannot be served over plaintext. If `"http2"` is included in `http.versions`, the transport | ||||||
| > security must be set to `"tls"` or `"mTLS"`. | ||||||
|
|
@@ -57,6 +57,10 @@ respective key prefix. | |||||
| | | `certificateVerificationMode` | `string` | Required for `"mTLS"`, permitted values: `"optionalVerification"`, `"noHostnameVerification"` | - | | ||||||
| | `backpressureStrategy` | `lowWatermark` | `int` | Optional | 2 | | ||||||
| | | `highWatermark` | `int` | Optional | 10 | | ||||||
| | - | `maxConnections` | `int` | Optional | nil | | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: there's a stray character here.
Suggested change
|
||||||
| | `connectionTimeouts` | `idle` | `int` | Optional | nil | | ||||||
| | | `readHeader` | `int` | Optional | nil | | ||||||
| | | `readBody` | `int` | Optional | nil | | ||||||
|
|
||||||
|
|
||||||
| The `credentialSource` determines how server credentials are provided: | ||||||
|
|
@@ -108,6 +112,12 @@ key were omitted. | |||||
| "backpressureStrategy": { | ||||||
| "lowWatermark": 2, // default: 2 | ||||||
| "highWatermark": 10 // default: 10 | ||||||
| }, | ||||||
| "maxConnections": 1000, // default: nil (unlimited) | ||||||
| "connectionTimeouts": { | ||||||
| "idle": 60, // default: nil (no timeout) | ||||||
| "readHeader": 30, // default: nil (no timeout) | ||||||
| "readBody": 60 // default: nil (no timeout) | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.