-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[binder] Handle outbound flow control #27243
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Seems like some inbound code are also in the outbound commit? Will review after inbound commit is merged |
877fb1a
to
1326f21
Compare
Sure, the inbound flow-control PR has been merged. |
c3ae8a0
to
034e539
Compare
grpc_core::MutexLock lock(&mu_); | ||
RETURN_IF_ERROR(binder_->PrepareTransaction()); | ||
WritableParcel* parcel = binder_->GetWritableParcel(); | ||
RETURN_IF_ERROR(parcel->WriteInt64(num_bytes)); | ||
return binder_->Transact(BinderTransportTxCode::ACKNOWLEDGE_BYTES); | ||
} | ||
|
||
void WireWriterImpl::RecvAck(int64_t num_bytes) { | ||
grpc_core::MutexLock lock(&mu_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no deadlock?
Is it because cv_.WaitWithDeadline
allows us to obtain mu_
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on a conditional variable will release the mutex IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. (I know it is true in boost, just want to confirm if that's also the case in abseil)
67228b0
to
840c703
Compare
840c703
to
9a8d312
Compare
Tested locally with Java servers.
Depends on #27228.