[binder] Handle outbound flow control#27243
Conversation
TaWeiTu
added
the
release notes: no
TaWeiTu
force-pushed
the
flow-control-outbound
branch
5 times, most recently
from
97e51fa to
877fb1a
Compare
|
Ping. |
| while (ptr < data.size()) { | ||
| while (num_outgoing_bytes_ >= | ||
| num_acknowledged_bytes_ + kFlowControlWindowSize) { | ||
| cv_.Wait(&mu_); |
There was a problem hiding this comment.
Probably shouldn't block here, it would be pretty hard to debug if we don't receive ACK from the other end for some reason (since the program will just hang).
We probably want a helper class or something to handle chunking so that flow control logic (which can be orthogonal to other logic) don't make WireWriterImpl become too complex
If that's too much to implement in the same PR, probably add a 10ms deadline to cv_.Wait() (and log ERROR if deadline exceeded) and add a TODO
There was a problem hiding this comment.
How about having a new class like FlowControl to take care of the flow control mechanism. The transport now holds a FlowControl (which internally holds a WieWtier to deal with the wire format) and the transport sends RPC call through FlowControl instead of WireWriter.
What do you think?
There was a problem hiding this comment.
I think that's probably too much abstraction? It should be possible to implement the flow control logic within WireWriterImpl and make the code readable? Probably introduce some kind of queue as member variable and some member functions
|
Seems like some inbound code are also in the outbound commit? Will review after inbound commit is merged |
TaWeiTu
force-pushed
the
flow-control-outbound
branch
from
877fb1a to
1326f21
Compare
Sure, the inbound flow-control PR has been merged. |
TaWeiTu
force-pushed
the
flow-control-outbound
branch
6 times, most recently
from
c3ae8a0 to
034e539
Compare
| } | ||
|
|
||
| void WireWriterImpl::RecvAck(int64_t num_bytes) { | ||
| grpc_core::MutexLock lock(&mu_); |
There was a problem hiding this comment.
there is no deadlock?
Is it because cv_.WaitWithDeadline allows us to obtain mu_ here?
There was a problem hiding this comment.
Waiting on a conditional variable will release the mutex IIRC.
There was a problem hiding this comment.
Ok. (I know it is true in boost, just want to confirm if that's also the case in abseil)
TaWeiTu
force-pushed
the
flow-control-outbound
branch
3 times, most recently
from
67228b0 to
840c703
Compare
TaWeiTu
force-pushed
the
flow-control-outbound
branch
from
840c703 to
9a8d312
Compare
2 participants
Tested locally with Java servers.
Depends on #27228.