Normalise config (#1906)

Motivation:

In 3272c32b the config for max idle time got moved into the connection
config for the server. As a follow up, the client should do the same.
While doing this it became more obvious that keepalive should also be
folded in to the connection config, as both relate to managing existing
connections. Moreover the server keepalive config can be a little
confusing because it includes config the server should use to keep the
connection alive and config the server should use to police how the
client is keeping the connection alive.

Modifications:

- Add a 'connection' config for the client and move the max idle time to
  it.
- Move the 'keepalive' config for client and server into their
  respective 'connection' configs
- Add a separate server keepalive config for policing the client
  keepalive and nest it in the server keepalive config

Result:

Easier to understand config

Sources/GRPCHTTP2Core/Client/Connection/GRPCChannel.swift

@@ -237,11 +237,8 @@ extension GRPCChannel {
     /// Configuration for backoff used when establishing a connection.
     var backoff: HTTP2ClientTransport.Config.Backoff
 
-    /// Configuration for dealing with idle connections.
-    var idle: HTTP2ClientTransport.Config.Idle?
-
-    /// Configuration for keepalive.
-    var keepalive: HTTP2ClientTransport.Config.Keepalive?
+    /// Configuration for connection management.
+    var connection: HTTP2ClientTransport.Config.Connection
 
     /// Compression configuration.
     var compression: HTTP2ClientTransport.Config.Compression
@@ -250,14 +247,12 @@ extension GRPCChannel {
     public init(
       http2: HTTP2ClientTransport.Config.HTTP2,
       backoff: HTTP2ClientTransport.Config.Backoff,
-      idle: HTTP2ClientTransport.Config.Idle?,
-      keepalive: HTTP2ClientTransport.Config.Keepalive?,
+      connection: HTTP2ClientTransport.Config.Connection,
       compression: HTTP2ClientTransport.Config.Compression
     ) {
       self.http2 = http2
       self.backoff = backoff
-      self.idle = idle
-      self.keepalive = keepalive
+      self.connection = connection
       self.compression = compression
     }
   }

Sources/GRPCHTTP2Core/Client/HTTP2ClientTransport.swift

@@ -62,29 +62,43 @@ extension HTTP2ClientTransport.Config {
     public var timeout: Duration
 
     /// Whether the client sends keepalive pings when there are no calls in progress.
-    public var permitWithoutCalls: Bool
+    public var allowWithoutCalls: Bool
 
     /// Creates a new keepalive configuration.
-    public init(time: Duration, timeout: Duration, permitWithoutCalls: Bool) {
+    public init(time: Duration, timeout: Duration, allowWithoutCalls: Bool) {
       self.time = time
       self.timeout = timeout
-      self.permitWithoutCalls = permitWithoutCalls
+      self.allowWithoutCalls = allowWithoutCalls
     }
   }
 
   @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
-  public struct Idle: Sendable {
+  public struct Connection: Sendable {
     /// The maximum amount of time a connection may be idle before it's closed.
-    public var maxTime: Duration
+    ///
+    /// Connections are considered idle when there are no open streams on them. Idle connections
+    /// can be closed after a configured amount of time to free resources. Note that servers may
+    /// separately monitor and close idle connections.
+    public var maxIdleTime: Duration?
+
+    /// Configuration for keepalive.
+    ///
+    /// Keepalive is typically applied to connection which have open streams. It can be useful to
+    /// detect dropped connections, particularly if the streams running on a connection don't have
+    /// much activity.
+    ///
+    /// See also: gRFC A8: Client-side Keepalive.
+    public var keepalive: Keepalive?
 
-    /// Creates an idle configuration.
-    public init(maxTime: Duration) {
-      self.maxTime = maxTime
+    /// Creates a connection configuration.
+    public init(maxIdleTime: Duration, keepalive: Keepalive?) {
+      self.maxIdleTime = maxIdleTime
+      self.keepalive = keepalive
     }
 
-    /// Default values, a 30 minute max idle time.
+    /// Default values, a 30 minute max idle time and no keepalive.
     public static var defaults: Self {
-      Self(maxTime: .seconds(30 * 60))
+      Self(maxIdleTime: .seconds(30 * 60), keepalive: nil)
     }
   }
 

Sources/GRPCHTTP2Core/Internal/NIOChannelPipeline+GRPC.swift

@@ -30,7 +30,6 @@ extension ChannelPipeline.SynchronousOperations {
   public func configureGRPCServerPipeline(
     channel: any Channel,
     compressionConfig: HTTP2ServerTransport.Config.Compression,
-    keepaliveConfig: HTTP2ServerTransport.Config.Keepalive,
     connectionConfig: HTTP2ServerTransport.Config.Connection,
     http2Config: HTTP2ServerTransport.Config.HTTP2,
     rpcConfig: HTTP2ServerTransport.Config.RPC,
@@ -41,10 +40,12 @@ extension ChannelPipeline.SynchronousOperations {
       maxIdleTime: connectionConfig.maxIdleTime.map { TimeAmount($0) },
       maxAge: connectionConfig.maxAge.map { TimeAmount($0) },
       maxGraceTime: connectionConfig.maxGraceTime.map { TimeAmount($0) },
-      keepaliveTime: TimeAmount(keepaliveConfig.time),
-      keepaliveTimeout: TimeAmount(keepaliveConfig.timeout),
-      allowKeepaliveWithoutCalls: keepaliveConfig.permitWithoutCalls,
-      minPingIntervalWithoutCalls: TimeAmount(keepaliveConfig.minPingIntervalWithoutCalls)
+      keepaliveTime: TimeAmount(connectionConfig.keepalive.time),
+      keepaliveTimeout: TimeAmount(connectionConfig.keepalive.timeout),
+      allowKeepaliveWithoutCalls: connectionConfig.keepalive.clientBehavior.allowWithoutCalls,
+      minPingIntervalWithoutCalls: TimeAmount(
+        connectionConfig.keepalive.clientBehavior.minPingIntervalWithoutCalls
+      )
     )
     let flushNotificationHandler = GRPCServerFlushNotificationHandler(
       serverConnectionManagementHandler: serverConnectionHandler
@@ -140,10 +141,10 @@ extension ChannelPipeline.SynchronousOperations {
 
     let connectionHandler = ClientConnectionHandler(
       eventLoop: self.eventLoop,
-      maxIdleTime: config.idle.map { TimeAmount($0.maxTime) },
-      keepaliveTime: config.keepalive.map { TimeAmount($0.time) },
-      keepaliveTimeout: config.keepalive.map { TimeAmount($0.timeout) },
-      keepaliveWithoutCalls: config.keepalive?.permitWithoutCalls ?? false
+      maxIdleTime: config.connection.maxIdleTime.map { TimeAmount($0) },
+      keepaliveTime: config.connection.keepalive.map { TimeAmount($0.time) },
+      keepaliveTimeout: config.connection.keepalive.map { TimeAmount($0.timeout) },
+      keepaliveWithoutCalls: config.connection.keepalive?.allowWithoutCalls ?? false
     )
 
     let multiplexer = try self.configureAsyncHTTP2Pipeline(

Sources/GRPCHTTP2Core/Server/HTTP2ServerTransport.swift

@@ -53,25 +53,18 @@ extension HTTP2ServerTransport.Config {
     /// The amount of time the server has to respond to a keepalive ping before the connection is closed.
     public var timeout: Duration
 
-    /// Whether the server allows the client to send keepalive pings when there are no calls in progress.
-    public var permitWithoutCalls: Bool
-
-    /// The minimum allowed interval the client is allowed to send keep-alive pings.
-    /// Pings more frequent than this interval count as 'strikes' and the connection is closed if there are
-    /// too many strikes.
-    public var minPingIntervalWithoutCalls: Duration
+    /// Configuration for how the server enforces client keepalive.
+    public var clientBehavior: ClientKeepaliveBehavior
 
     /// Creates a new keepalive configuration.
     public init(
       time: Duration,
       timeout: Duration,
-      permitWithoutCalls: Bool,
-      minPingIntervalWithoutCalls: Duration
+      clientBehavior: ClientKeepaliveBehavior
     ) {
       self.time = time
       self.timeout = timeout
-      self.permitWithoutCalls = permitWithoutCalls
-      self.minPingIntervalWithoutCalls = minPingIntervalWithoutCalls
+      self.clientBehavior = clientBehavior
     }
 
     /// Default values. The time after reading data a ping should be sent defaults to 2 hours, the timeout for
@@ -81,12 +74,38 @@ extension HTTP2ServerTransport.Config {
       Self(
         time: .seconds(2 * 60 * 60),  // 2 hours
         timeout: .seconds(20),
-        permitWithoutCalls: false,
-        minPingIntervalWithoutCalls: .seconds(5 * 60)  // 5 minutes
+        clientBehavior: .defaults
       )
     }
   }
 
+  @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
+  public struct ClientKeepaliveBehavior: Sendable {
+    /// The minimum allowed interval the client is allowed to send keep-alive pings.
+    /// Pings more frequent than this interval count as 'strikes' and the connection is closed if there are
+    /// too many strikes.
+    public var minPingIntervalWithoutCalls: Duration
+
+    /// Whether the server allows the client to send keepalive pings when there are no calls in progress.
+    public var allowWithoutCalls: Bool
+
+    /// Creates a new configuration for permitted client keepalive behavior.
+    public init(
+      minPingIntervalWithoutCalls: Duration,
+      allowWithoutCalls: Bool
+    ) {
+      self.minPingIntervalWithoutCalls = minPingIntervalWithoutCalls
+      self.allowWithoutCalls = allowWithoutCalls
+    }
+
+    /// Default values. The time after reading data a ping should be sent defaults to 2 hours, the timeout for
+    /// keepalive pings defaults to 20 seconds, pings are not permitted when no calls are in progress, and
+    /// the minimum allowed interval for clients to send pings defaults to 5 minutes.
+    public static var defaults: Self {
+      Self(minPingIntervalWithoutCalls: .seconds(5 * 60), allowWithoutCalls: false)
+    }
+  }
+
   @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
   public struct Connection: Sendable {
     /// The maximum amount of time a connection may exist before being gracefully closed.
@@ -98,19 +117,28 @@ extension HTTP2ServerTransport.Config {
     /// The maximum amount of time a connection may be idle before it's closed.
     public var maxIdleTime: Duration?
 
+    /// Configuration for keepalive used to detect broken connections.
+    ///
+    /// - SeeAlso: gRFC A8 for client side keepalive, and gRFC A9 for server connection management.
+    public var keepalive: Keepalive
+
     public init(
       maxAge: Duration?,
       maxGraceTime: Duration?,
-      maxIdleTime: Duration?
+      maxIdleTime: Duration?,
+      keepalive: Keepalive
     ) {
       self.maxAge = maxAge
       self.maxGraceTime = maxGraceTime
       self.maxIdleTime = maxIdleTime
+      self.keepalive = keepalive
     }
 
-    /// Default values. All the max connection age, max grace time, and max idle time default to infinite.
+    /// Default values. The max connection age, max grace time, and max idle time default to
+    /// `nil` (i.e. infinite). See ``HTTP2ServerTransport/Config/Keepalive/defaults`` for keepalive
+    /// defaults.
     public static var defaults: Self {
-      Self(maxAge: nil, maxGraceTime: nil, maxIdleTime: nil)
+      Self(maxAge: nil, maxGraceTime: nil, maxIdleTime: nil, keepalive: .defaults)
     }
   }
 

Sources/GRPCHTTP2TransportNIOPosix/GRPCHTTP2TransportNIOPosix.swift

@@ -54,7 +54,6 @@ extension HTTP2ServerTransport {
             return try channel.pipeline.syncOperations.configureGRPCServerPipeline(
               channel: channel,
               compressionConfig: self.config.compression,
-              keepaliveConfig: self.config.keepalive,
               connectionConfig: self.config.connection,
               http2Config: self.config.http2,
               rpcConfig: self.config.rpc,
@@ -135,8 +134,6 @@ extension HTTP2ServerTransport.Posix {
   public struct Config: Sendable {
     /// Compression configuration.
     public var compression: HTTP2ServerTransport.Config.Compression
-    /// Keepalive configuration.
-    public var keepalive: HTTP2ServerTransport.Config.Keepalive
     /// Connection configuration.
     public var connection: HTTP2ServerTransport.Config.Connection
     /// HTTP2 configuration.
@@ -147,19 +144,16 @@ extension HTTP2ServerTransport.Posix {
     /// Construct a new `Config`.
     /// - Parameters:
     ///   - compression: Compression configuration.
-    ///   - keepalive: Keepalive configuration.
     ///   - connection: Connection configuration.
     ///   - http2: HTTP2 configuration.
     ///   - rpc: RPC configuration.
     public init(
       compression: HTTP2ServerTransport.Config.Compression,
-      keepalive: HTTP2ServerTransport.Config.Keepalive,
       connection: HTTP2ServerTransport.Config.Connection,
       http2: HTTP2ServerTransport.Config.HTTP2,
       rpc: HTTP2ServerTransport.Config.RPC
     ) {
       self.compression = compression
-      self.keepalive = keepalive
       self.connection = connection
       self.http2 = http2
       self.rpc = rpc
@@ -169,7 +163,6 @@ extension HTTP2ServerTransport.Posix {
     public static var defaults: Self {
       Self(
         compression: .defaults,
-        keepalive: .defaults,
         connection: .defaults,
         http2: .defaults,
         rpc: .defaults

Tests/GRPCHTTP2CoreTests/Client/Connection/GRPCChannelTests.swift

@@ -555,8 +555,7 @@ extension GRPCChannel.Config {
     Self(
       http2: .defaults,
       backoff: .defaults,
-      idle: .defaults,
-      keepalive: nil,
+      connection: .defaults,
       compression: .defaults
     )
   }

Tests/GRPCHTTP2CoreTests/Client/HTTP2ClientTransportConfigTests.swift

@@ -25,9 +25,10 @@ final class HTTP2ClientTransportConfigTests: XCTestCase {
     XCTAssertEqual(config.enabledAlgorithms, .none)
   }
 
-  func testIdleDefaults() {
-    let config = HTTP2ClientTransport.Config.Idle.defaults
-    XCTAssertEqual(config.maxTime, .seconds(30 * 60))
+  func testConnectionDefaults() {
+    let config = HTTP2ClientTransport.Config.Connection.defaults
+    XCTAssertEqual(config.maxIdleTime, .seconds(30 * 60))
+    XCTAssertNil(config.keepalive)
   }
 
   func testBackoffDefaults() {

Tests/GRPCHTTP2CoreTests/Server/HTTP2ServerTransportConfigTests.swift

@@ -28,8 +28,8 @@ final class HTTP2ServerTransportConfigTests: XCTestCase {
     let config = HTTP2ServerTransport.Config.Keepalive.defaults
     XCTAssertEqual(config.time, .seconds(7200))
     XCTAssertEqual(config.timeout, .seconds(20))
-    XCTAssertEqual(config.permitWithoutCalls, false)
-    XCTAssertEqual(config.minPingIntervalWithoutCalls, .seconds(300))
+    XCTAssertEqual(config.clientBehavior.allowWithoutCalls, false)
+    XCTAssertEqual(config.clientBehavior.minPingIntervalWithoutCalls, .seconds(300))
   }
 
   func testConnectionDefaults() {