From b7df4c8290be2ce164b5820bca908419919a0b09 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 22 Jun 2023 10:30:53 -0400 Subject: [PATCH] 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 | 1 + lib/src/client/http2_connection.dart | 6 +- test/keepalive_test.dart | 14 +-- test/server_keepalive_manager_test.dart | 113 +++++++++++++----------- 4 files changed, 76 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e208902..dda4842 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/src/client/http2_connection.dart b/lib/src/client/http2_connection.dart index 0f1a40d..5f76f5e 100644 --- a/lib/src/client/http2_connection.dart +++ b/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 diff --git a/test/keepalive_test.dart b/test/keepalive_test.dart index faeefba..51ef15e 100644 --- a/test/keepalive_test.dart +++ b/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 diff --git a/test/server_keepalive_manager_test.dart b/test/server_keepalive_manager_test.dart index 34f0afb..230fc3b 100644 --- a/test/server_keepalive_manager_test.dart +++ b/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); + }); }); }