Multiplex RPCs on a single connection. #34
Conversation
A Channel will now multiplex RPCs on a single managed connection. The connection will attempt to reconnect on failure, and will close the underlying transport connection if no RPCs have been made for a while. Part of grpc#5.
test/client_test.dart
Outdated
| harness.signalIdle(); | ||
| expect( | ||
| connectionStates, [ConnectionState.connecting, ConnectionState.ready]); | ||
| await new Future.delayed(const Duration(microseconds: 100)); |
There was a problem hiding this comment.
You could complete a Completer in your onStateChanged callback when ConnectionState.idle appears. Then the test does not need an arbitrary delay.
There was a problem hiding this comment.
Done. I kind of wanted to make sure I didn't get another state change after idle, but I don't really think it matters.
| {List<int> certificate, String password, String authority}) | ||
| : this._(true, certificate, password, authority); | ||
| const ChannelOptions.insecure( | ||
| {Duration idleTimeout, |
There was a problem hiding this comment.
Is this formatting done by dartfmt?
There was a problem hiding this comment.
Hard to believe, but yes. It insists. (And the Travis checks enforce using dartfmt.)
| } | ||
| return context; | ||
| } | ||
| } | ||
|
|
||
| enum ConnectionState { |
There was a problem hiding this comment.
The enum constants should ideally use the same grammatical form. Here, connecting/ready/idle use one form, whereas transientFailure uses another, and shutdown can be read in two different ways, one of which is a command. Maybe connecting/ready/failed/idle/down?
There was a problem hiding this comment.
I agree, but these names are defined by gRPC: https://github.com/grpc/grpc/blob/master/doc/connectivity-semantics-and-api.md
lib/src/client.dart
Outdated
|
|
||
| void _handleIdleTimeout() { | ||
| if (_timer == null) return; | ||
| _timer = null; |
There was a problem hiding this comment.
Use _cancelTimer instead of setting _timer to null in more than one place. Conveys intent better, I think. Plus it cancels unwanted timer events instead of attempting to ignore them when they occur. Unwanted timer events can make debugging hard, but I think your attempt at ignoring them may even fail, if a new timer has been set up in the mean time? You may need to receive the timer object with timer events and compare to current _timer before reacting.
There was a problem hiding this comment.
I've added in a check for the current state. Idle timeout should only fire while in the ready state. We only set the timeout when there are no more active streams, and we explicitly clear it when the connection is active again, so if we get a timeout callback, and:
- _state == ready
- _timer != null
then at least there are no active streams. Worst case, we end up closing an idle connection too soon, but the following would have to happen:
- Connection has been idle long enough for the timer to fire and schedule an event to call
_handleTimeout(). - Before
_handleTimeout()is actually called, the currently running code makes an RPC, making the connection active (and clearing_timerin the process). - Still before the scheduled
_handleTimeout()call is invoked, the RPC manages to finish, and a new idle timeout is scheduled. - The original
_handleTimeout()call finally runs, and closes the connection.
I don't think it's possible to complete an RPC without processing other events in the queue (especially since the onActiveStateChange call to mark the transport idle is delivered in a Future event, which would come later in the queue), so I we should be good now.
lib/src/client.dart
Outdated
| // TODO(jakobr): Log error. | ||
| _cancelTimer(); | ||
| if (!_hasPendingCalls()) { | ||
| // If there are no pending calls, just go directly to idle. |
There was a problem hiding this comment.
[nit] This comment just repeats what the code already says.
There was a problem hiding this comment.
True, it does now. Removed.
A Channel will now multiplex RPCs on a single managed connection. The
connection will attempt to reconnect on failure, and will close the
underlying transport connection if no RPCs have been made for a while.
Part of #5.