Skip to content

Commit

Permalink
fix: keep alive timeout finishes transport instead of connection shut…
Browse files Browse the repository at this point in the history
…down (#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>
  • Loading branch information
steffenhaak and mosuem authored Sep 6, 2024
1 parent 8177633 commit 071ebc5
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 11 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion lib/src/client/http2_connection.dart
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class Http2ClientConnection implements connection.ClientConnection {
transport.ping();
}
},
onPingTimeout: () => shutdown(),
onPingTimeout: () => transport.finish(),
);
transport.onFrameReceived
.listen((_) => keepAliveManager?.onFrameReceived());
Expand Down
27 changes: 17 additions & 10 deletions test/keepalive_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});
}

Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down

0 comments on commit 071ebc5

Please sign in to comment.