-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix deadlock in testStreamAckDeadlineUpdate #1581
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
Fixes #1577. The detailed description of the bug is available in the issue. This commit fixes the bug by ensuring that the Future received by MessageReceiver cannot be completed before a callback is added to it.
LGTM, but let @davidtorres take a look too. |
*/ | ||
ListenableFuture<AckReply> receiveMessage(PubsubMessage message); | ||
void receiveMessage(PubsubMessage message, SettableFuture<AckReply> response); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@garrettjonesgoogle is right that adding |
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.
LGTM
cc @davidtorres
Fixes #1577. The detailed description of the bug is available in the
issue.
This commit fixes the bug by ensuring that the Future received by
MessageReceiver cannot be completed before a callback is added to it.
@garrettjonesgoogle I know that we are moving away from exposing Guava types. Since this branch has not been moved to
RpcFuture
yet, I think it's best to keep using Guava types for now. I'm mostly afraid that moving types might change the timing just enough that this bug become more difficult to reproduce.