-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Respect requested message limits within a single MessageProducer in BinderTransport. #9163
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
Conversation
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.
Looks good to me, but wait for ejona since the new test affects other transport as well.
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 believe flowControlPushBack()
would have ordinarily caught this bug, but that test assumes end-to-end flow control which isn't in-place for Binder so the test is disabled. This new test is inherently timing-sensitive (just like flowControlPushBack()
) and might not catch bugs. @akandratovich, did you confirm the test fails before the bug fix?
@ejona86: I verified it failed before the fix, but that's a good point about timing. It might help to have the client halfClose, and then awaitHalfClose on the server. |
Sorry, awaitHalfClose won't help, but adding a doPingPong() call should help. |
Ah, yes. |
I tried enabling |
Add check for request message count in binder grpc inbound stream.
Without check if messages are available and requested, producer provides everything that is available ignoring the request count. It leads to call listener API violation - it's possible that more messages will be provided than requested.