Improve connection handling #231

sigurdm · 2019-08-16T14:26:21Z

Most of this is appropriated from the grpc implementation we had in https://github.com/dart-lang/appengine. Big thanks to @mkustermann for exlpaining the intricate details!

This should make the connection handling a lot more robust, and also faster.
Hopefully dealing with: #228

tvolkert · 2019-08-18T07:04:38Z

lib/src/client/http2_connection.dart

+    if (securityContext == null) {
+      socket = await Socket.connect(host, port);
+    } else {
+      final socket = await SecureSocket.connect(host, port,


You're creating a new local variable here, so the outer socket remains null. Perhaps name this secureSocket, and then set socket = secureSocket after the "h2" if check below?

(also, is there any way to test this code path?)

Oops
Thanks for catching this!

I added a simple round trip test.

The interop tests would also have caught this. They run on master.
They should be set up to run on travis for pull requests. I have got some hints how to do this, but never got around to it :(.

domesticmouse

Lgtm

tvolkert · 2019-08-19T02:41:59Z

lib/src/client/http2_connection.dart

+    socket.done.then((_) => _abandonConnection());
+
+    // Give the settings settings-frame a bit of time to arrive.
+    await new Future.delayed(_estimatedRoundTripTime);


This seems like a smell - can we not be notified somehow when we're ready?

This is indeed a hack.
I don't think the http2 package currently exposes the possibility of waiting for the settings frame. Not sure we are even guaranteed to have one.
I'll add a TODO reminding us of the smell.
@mkustermann might have more insight.

Maybe add a comment that explains why we do this, and how it could be better.

tvolkert · 2019-08-19T02:47:20Z

lib/src/client/http2_connection.dart

+    final bool shouldRefresh =
+        _connectionLifeTimer.elapsed > options.connectionTimeout;
+    if (!isHealthy || shouldRefresh) {
+      if (isHealthy) {


This is the same as if (shouldRefresh), and there's no reason for it to be in the outer if block.

True - that reads simpler.
Thanks!

tvolkert · 2019-08-19T05:47:23Z

The old appengine code used a Dialer to ensure only one new http/2 connection is made if multiple event loops try to establish a new connection. Does this handle that case?

jonasfj · 2019-08-19T08:47:03Z

pubspec.yaml

@@ -1,7 +1,7 @@
 name: grpc
 description: Dart implementation of gRPC, a high performance, open-source universal RPC framework.

-version: 2.0.3
+version: 2.0.4


Considering the new options added to the public API, it might be fine to do a minor version bump.

Right!
Done!

jonasfj · 2019-08-19T10:33:02Z

lib/src/client/http2_connection.dart

+    socket.done.then((_) => _abandonConnection());
+
+    // Give the settings settings-frame a bit of time to arrive.
+    await new Future.delayed(_estimatedRoundTripTime);


Maybe add a comment that explains why we do this, and how it could be better.

sigurdm

Thanks for the reviews!

sigurdm · 2019-08-19T07:26:01Z

lib/src/client/http2_connection.dart

+    socket.done.then((_) => _abandonConnection());
+
+    // Give the settings settings-frame a bit of time to arrive.
+    await new Future.delayed(_estimatedRoundTripTime);


This is indeed a hack.
I don't think the http2 package currently exposes the possibility of waiting for the settings frame. Not sure we are even guaranteed to have one.
I'll add a TODO reminding us of the smell.
@mkustermann might have more insight.

sigurdm · 2019-08-19T07:28:03Z

lib/src/client/http2_connection.dart

+    final bool shouldRefresh =
+        _connectionLifeTimer.elapsed > options.connectionTimeout;
+    if (!isHealthy || shouldRefresh) {
+      if (isHealthy) {


True - that reads simpler.
Thanks!

sigurdm · 2019-08-19T10:57:36Z

lib/src/client/http2_connection.dart

+    if (securityContext == null) {
+      socket = await Socket.connect(host, port);
+    } else {
+      final socket = await SecureSocket.connect(host, port,


I added a simple round trip test.

The interop tests would also have caught this. They run on master.
They should be set up to run on travis for pull requests. I have got some hints how to do this, but never got around to it :(.

sigurdm · 2019-08-19T10:58:36Z

pubspec.yaml

@@ -1,7 +1,7 @@
 name: grpc
 description: Dart implementation of gRPC, a high performance, open-source universal RPC framework.

-version: 2.0.3
+version: 2.0.4


Right!
Done!

tvolkert · 2019-08-19T19:31:11Z

Thanks @sigurdm - can you publish this to Pub too?

sigurdm · 2019-08-20T07:18:31Z

Yep - done

Improve connection handling

5d9c9e4

sigurdm requested a review from jonasfj August 16, 2019 14:26

googlebot added the cla: yes label Aug 16, 2019

sigurdm mentioned this pull request Aug 16, 2019

Proposal: FlutterClientChannel for handling mobile connection state #223

Closed

tvolkert mentioned this pull request Aug 18, 2019

Not properly handling http/2 connection becoming not healthy #228

Closed

tvolkert reviewed Aug 18, 2019

View reviewed changes

domesticmouse approved these changes Aug 19, 2019

View reviewed changes

tvolkert reviewed Aug 19, 2019

View reviewed changes

jonasfj approved these changes Aug 19, 2019

View reviewed changes

Address review. Add round-trip-test

423e0ce

sigurdm commented Aug 19, 2019

View reviewed changes

Merge branch 'master' into check_http2_connection_health

3e1aa0b

sigurdm merged commit 992e2dc into grpc:master Aug 19, 2019

sigurdm deleted the check_http2_connection_health branch August 20, 2019 07:18

sigurdm mentioned this pull request Aug 22, 2019

TEST FAILURE: grpc_interop_tocloud failed with CERTIFICATE_VERIFY_FAILED grpc/grpc#20017

Closed

lucaslcode mentioned this pull request Jun 25, 2020

Question about restore connection #180

Closed

haf mentioned this pull request Feb 29, 2024

Reconnect logic #691

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve connection handling #231

Improve connection handling #231

sigurdm commented Aug 16, 2019

tvolkert Aug 18, 2019

tvolkert Aug 18, 2019

sigurdm Aug 19, 2019

sigurdm Aug 19, 2019

domesticmouse left a comment

tvolkert Aug 19, 2019

sigurdm Aug 19, 2019

jonasfj Aug 19, 2019

tvolkert Aug 19, 2019

sigurdm Aug 19, 2019

tvolkert commented Aug 19, 2019

jonasfj Aug 19, 2019

sigurdm Aug 19, 2019

jonasfj Aug 19, 2019

sigurdm left a comment

sigurdm Aug 19, 2019

sigurdm Aug 19, 2019

sigurdm Aug 19, 2019

sigurdm Aug 19, 2019

tvolkert commented Aug 19, 2019

sigurdm commented Aug 20, 2019

Improve connection handling #231

Improve connection handling #231

Conversation

sigurdm commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domesticmouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvolkert commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigurdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvolkert commented Aug 19, 2019

sigurdm commented Aug 20, 2019