From 071ebc5f31a18ab52e82c09558f3dcb85f41fdbd Mon Sep 17 00:00:00 2001
From: steffenhaak <haak@aucentiq.com>
Date: Fri, 6 Sep 2024 17:13:11 +0200
Subject: [PATCH] fix: keep alive timeout finishes transport instead of
 connection shutdown (#722)

* fix: keep alive timeout finishes transport instead of shutting down channel

* Update keepalive_test.dart

* Update CHANGELOG.md

---------

Co-authored-by: Moritz <mosum@google.com>
---
 CHANGELOG.md                         |  2 ++
 lib/src/client/http2_connection.dart |  2 +-
 test/keepalive_test.dart             | 27 +++++++++++++++++----------
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 14b3032..0f463fb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 * Internal optimization to client code.
 * Small fixes, such as ports in testing and enabling `timeline_test.dart`.
+* When the keep alive manager runs into a timeout, it will finish the transport instead of closing
+  the connection, as defined in the gRPC spec.
 
 ## 4.0.1
 
diff --git a/lib/src/client/http2_connection.dart b/lib/src/client/http2_connection.dart
index 998410a..4cce677 100644
--- a/lib/src/client/http2_connection.dart
+++ b/lib/src/client/http2_connection.dart
@@ -113,7 +113,7 @@ class Http2ClientConnection implements connection.ClientConnection {
               transport.ping();
             }
           },
-          onPingTimeout: () => shutdown(),
+          onPingTimeout: () => transport.finish(),
         );
         transport.onFrameReceived
             .listen((_) => keepAliveManager?.onFrameReceived());
diff --git a/test/keepalive_test.dart b/test/keepalive_test.dart
index 3da1b80..bf48f9b 100644
--- a/test/keepalive_test.dart
+++ b/test/keepalive_test.dart
@@ -32,7 +32,7 @@ void main() {
   late EchoServiceClient fakeClient;
   late FakeClientChannel fakeChannel;
   late EchoServiceClient unresponsiveClient;
-  late ClientChannel unresponsiveChannel;
+  late FakeClientChannel unresponsiveChannel;
 
   final pingInterval = Duration(milliseconds: 10);
   final timeout = Duration(milliseconds: 30);
@@ -101,14 +101,18 @@ void main() {
     expect(fakeChannel.newConnectionCounter, 1);
   });
 
-  test('Server doesnt ack the ping, making the client shutdown the connection',
+  test('Server doesnt ack the ping, making the client shutdown the transport',
       () async {
+    //Send a first request, get a connection
     await unresponsiveClient.echo(EchoRequest());
+    expect(unresponsiveChannel.newConnectionCounter, 1);
+
+    //Ping is not being acked on time
     await Future.delayed(timeout * 2);
-    await expectLater(
-      unresponsiveClient.echo(EchoRequest()),
-      throwsA(isA<GrpcError>()),
-    );
+
+    //A second request gets a new connection
+    await unresponsiveClient.echo(EchoRequest());
+    expect(unresponsiveChannel.newConnectionCounter, 2);
   });
 }
 
@@ -146,7 +150,7 @@ class FakeHttp2ClientConnection extends Http2ClientConnection {
 }
 
 /// A wrapper around a [FakeHttp2ClientConnection]
-class UnresponsiveClientChannel extends ClientChannel {
+class UnresponsiveClientChannel extends FakeClientChannel {
   UnresponsiveClientChannel(
     super.host, {
     super.port,
@@ -155,11 +159,14 @@ class UnresponsiveClientChannel extends ClientChannel {
   });
 
   @override
-  ClientConnection createConnection() =>
-      UnresponsiveHttp2ClientConnection(host, port, options);
+  ClientConnection createConnection() {
+    fakeHttp2ClientConnection =
+        UnresponsiveHttp2ClientConnection(host, port, options);
+    return fakeHttp2ClientConnection!;
+  }
 }
 
-class UnresponsiveHttp2ClientConnection extends Http2ClientConnection {
+class UnresponsiveHttp2ClientConnection extends FakeHttp2ClientConnection {
   UnresponsiveHttp2ClientConnection(super.host, super.port, super.options);
 
   @override