Deflake keepalive tests (#645)

* Deflake keepalive tests

* Add timer

* Increase wait timer

* Only send ping on open connection

* Switch server keepalive to fakeasync

* Tiny refactor

* Add changelog entry

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 * Add const constructor to `GrpcError` fixing #606.
 * Make `GrpcError` non-final to allow implementations.
+* Only send keepalive pings on open connections.
 
 ## 3.2.2
 

lib/src/client/http2_connection.dart

@@ -106,7 +106,11 @@ class Http2ClientConnection implements connection.ClientConnection {
       if (options.keepAlive.shouldSendPings) {
         keepAliveManager = ClientKeepAlive(
           options: options.keepAlive,
-          ping: () => transport.ping(),
+          ping: () {
+            if (transport.isOpen) {
+              transport.ping();
+            }
+          },
           onPingTimeout: () => shutdown(),
         );
         transport.onFrameReceived

test/keepalive_test.dart

@@ -56,7 +56,7 @@ void main() {
   });
 
   tearDown(() async {
-    await fakeChannel.terminate();
+    await fakeChannel.shutdown();
     await server.shutdown();
   });
 
@@ -73,8 +73,12 @@ void main() {
   test('Server doesnt terminate connection after pings, as data is sent',
       () async {
     final timer = Timer.periodic(
-        Duration(milliseconds: 30), (timer) => fakeClient.echo(EchoRequest()));
+        Duration(milliseconds: 10), (timer) => fakeClient.echo(EchoRequest()));
     await Future.delayed(Duration(milliseconds: 200), () => timer.cancel());
+
+    // Wait for last request to be sent
+    await Future.delayed(Duration(milliseconds: 20));
+
     // Check that the server never closed the connection
     expect(fakeChannel.newConnectionCounter, 1);
   });
@@ -90,7 +94,7 @@ void main() {
 
 /// A wrapper around a [FakeHttp2ClientConnection]
 class FakeClientChannel extends ClientChannel {
-  late FakeHttp2ClientConnection fakeHttp2ClientConnection;
+  FakeHttp2ClientConnection? fakeHttp2ClientConnection;
   FakeClientChannel(
     super.host, {
     super.port = 443,
@@ -101,11 +105,11 @@ class FakeClientChannel extends ClientChannel {
   @override
   ClientConnection createConnection() {
     fakeHttp2ClientConnection = FakeHttp2ClientConnection(host, port, options);
-    return fakeHttp2ClientConnection;
+    return fakeHttp2ClientConnection!;
   }
 
   int get newConnectionCounter =>
-      fakeHttp2ClientConnection.newConnectionCounter;
+      fakeHttp2ClientConnection?.newConnectionCounter ?? 0;
 }
 
 /// A [Http2ClientConnection] exposing a counter for new connections

test/server_keepalive_manager_test.dart

@@ -1,5 +1,6 @@
 import 'dart:async';
 
+import 'package:fake_async/fake_async.dart';
 import 'package:grpc/src/server/server_keepalive.dart';
 import 'package:test/test.dart';
 
@@ -33,75 +34,83 @@ void main() {
     dataStream.close();
   });
 
+  final timeAfterPing = Duration(milliseconds: 10);
+
   test('Sending too many pings without data kills connection', () async {
-    initServer();
-    // Send good ping
-    pingStream.sink.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
-
-    // Send [maxBadPings] bad pings, that's still ok
-    for (var i = 0; i < maxBadPings; i++) {
+    FakeAsync().run((async) {
+      initServer();
+      // Send good ping
       pingStream.sink.add(null);
-    }
-    await Future.delayed(Duration(milliseconds: 10));
-    expect(goAway, false);
+      async.elapse(timeAfterPing);
 
-    // Send another bad ping; that's one too many!
-    pingStream.sink.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
-    expect(goAway, true);
+      // Send [maxBadPings] bad pings, that's still ok
+      for (var i = 0; i < maxBadPings; i++) {
+        pingStream.sink.add(null);
+      }
+      async.elapse(timeAfterPing);
+      expect(goAway, false);
+
+      // Send another bad ping; that's one too many!
+      pingStream.sink.add(null);
+      async.elapse(timeAfterPing);
+      expect(goAway, true);
+    });
   });
   test(
       'Sending too many pings without data doesn`t kill connection if the server doesn`t care',
       () async {
-    initServer(ServerKeepAliveOptions(
-      maxBadPings: null,
-      minIntervalBetweenPingsWithoutData: Duration(milliseconds: 5),
-    ));
-    // Send good ping
-    pingStream.sink.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
-
-    // Send a lot of bad pings, that's still ok.
-    for (var i = 0; i < 50; i++) {
+    FakeAsync().run((async) {
+      initServer(ServerKeepAliveOptions(
+        maxBadPings: null,
+        minIntervalBetweenPingsWithoutData: Duration(milliseconds: 5),
+      ));
+      // Send good ping
       pingStream.sink.add(null);
-    }
-    await Future.delayed(Duration(milliseconds: 10));
-    expect(goAway, false);
+      async.elapse(timeAfterPing);
+
+      // Send a lot of bad pings, that's still ok.
+      for (var i = 0; i < 50; i++) {
+        pingStream.sink.add(null);
+      }
+      async.elapse(timeAfterPing);
+      expect(goAway, false);
+    });
   });
 
   test('Sending many pings with data doesn`t kill connection', () async {
-    initServer();
+    FakeAsync().run((async) {
+      initServer();
 
-    // Send good ping
-    pingStream.sink.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
-
-    // Send [maxBadPings] bad pings, that's still ok
-    for (var i = 0; i < maxBadPings; i++) {
+      // Send good ping
       pingStream.sink.add(null);
-    }
-    await Future.delayed(Duration(milliseconds: 10));
-    expect(goAway, false);
+      async.elapse(timeAfterPing);
 
-    // Sending data resets the bad ping count
-    dataStream.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
+      // Send [maxBadPings] bad pings, that's still ok
+      for (var i = 0; i < maxBadPings; i++) {
+        pingStream.sink.add(null);
+      }
+      async.elapse(timeAfterPing);
+      expect(goAway, false);
 
-    // Send good ping
-    pingStream.sink.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
+      // Sending data resets the bad ping count
+      dataStream.add(null);
+      async.elapse(timeAfterPing);
 
-    // Send [maxBadPings] bad pings, that's still ok
-    for (var i = 0; i < maxBadPings; i++) {
+      // Send good ping
       pingStream.sink.add(null);
-    }
-    await Future.delayed(Duration(milliseconds: 10));
-    expect(goAway, false);
+      async.elapse(timeAfterPing);
 
-    // Send another bad ping; that's one too many!
-    pingStream.sink.add(null);
-    await Future.delayed(Duration(milliseconds: 10));
-    expect(goAway, true);
+      // Send [maxBadPings] bad pings, that's still ok
+      for (var i = 0; i < maxBadPings; i++) {
+        pingStream.sink.add(null);
+      }
+      async.elapse(timeAfterPing);
+      expect(goAway, false);
+
+      // Send another bad ping; that's one too many!
+      pingStream.sink.add(null);
+      async.elapse(timeAfterPing);
+      expect(goAway, true);
+    });
   });
 }