Start the idle timer when the connection is ready (#1915)

Motivation:

Some of the idle tests are flaky because the idle timer races the
connection becoming ready which can lead to state transitions the tests
aren't expecting. The root of this is caused by the idle timer starting
on channel active instead of when the connection becomes ready.

Modification:

- Only start the idle/keepalive timers when the connection becomes ready

Result:

More reliable tests

Sources/GRPCHTTP2Core/Client/Connection/ClientConnectionHandler.swift

@@ -130,16 +130,6 @@ final class ClientConnectionHandler: ChannelInboundHandler, ChannelOutboundHandl
     self.context = nil
   }
 
-  func channelActive(context: ChannelHandlerContext) {
-    self.keepaliveTimer?.schedule(on: context.eventLoop) {
-      self.keepaliveTimerFired(context: context)
-    }
-
-    self.maxIdleTimer?.schedule(on: context.eventLoop) {
-      self.maxIdleTimerFired(context: context)
-    }
-  }
-
   func channelInactive(context: ChannelHandlerContext) {
     switch self.state.closed() {
     case .none:
@@ -221,6 +211,14 @@ final class ClientConnectionHandler: ChannelInboundHandler, ChannelOutboundHandl
       // becoming active is insufficient as, for example, a TLS handshake may fail after
       // establishing the TCP connection, or the server isn't configured for gRPC (or HTTP/2).
       if isInitialSettings {
+        self.keepaliveTimer?.schedule(on: context.eventLoop) {
+          self.keepaliveTimerFired(context: context)
+        }
+
+        self.maxIdleTimer?.schedule(on: context.eventLoop) {
+          self.maxIdleTimerFired(context: context)
+        }
+
         context.fireChannelRead(self.wrapInboundOut(.ready))
       }
 

Tests/GRPCHTTP2CoreTests/Client/Connection/ClientConnectionHandlerTests.swift

@@ -26,6 +26,10 @@ final class ClientConnectionHandlerTests: XCTestCase {
     let connection = try Connection(maxIdleTime: .minutes(5))
     try connection.activate()
 
+    // Write the initial settings to ready the connection.
+    try connection.settings([])
+    XCTAssertEqual(try connection.readEvent(), .ready)
+
     // Idle with no streams open we should:
     // - read out a closing event,
     // - write a GOAWAY frame,
@@ -74,6 +78,10 @@ final class ClientConnectionHandlerTests: XCTestCase {
     let connection = try Connection(keepaliveTime: .minutes(1), keepaliveTimeout: .seconds(10))
     try connection.activate()
 
+    // Write the initial settings to ready the connection.
+    try connection.settings([])
+    XCTAssertEqual(try connection.readEvent(), .ready)
+
     // Open a stream so keep-alive starts.
     connection.streamOpened(1)
 
@@ -100,6 +108,10 @@ final class ClientConnectionHandlerTests: XCTestCase {
     let connection = try Connection(keepaliveTime: .minutes(1), allowKeepaliveWithoutCalls: true)
     try connection.activate()
 
+    // Write the initial settings to ready the connection.
+    try connection.settings([])
+    XCTAssertEqual(try connection.readEvent(), .ready)
+
     for _ in 0 ..< 10 {
       // Advance time, a PING should be sent, ACK it.
       connection.loop.advanceTime(by: .minutes(1))
@@ -118,6 +130,10 @@ final class ClientConnectionHandlerTests: XCTestCase {
     let connection = try Connection(keepaliveTime: .minutes(1), keepaliveTimeout: .seconds(10))
     try connection.activate()
 
+    // Write the initial settings to ready the connection.
+    try connection.settings([])
+    XCTAssertEqual(try connection.readEvent(), .ready)
+
     // Open a stream so keep-alive starts.
     connection.streamOpened(1)