Skip to content

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

Merged
merged 2 commits into from
Feb 2, 2017
Merged

fix deadlock in testStreamAckDeadlineUpdate #1581

merged 2 commits into from
Feb 2, 2017

Conversation

pongad
Copy link
Contributor

@pongad pongad commented Feb 1, 2017

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.

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.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 1, 2017
@garrettjonesgoogle
Copy link
Member

LGTM, but let @davidtorres take a look too.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 83.085% when pulling 7e5b51e on pongad:fix-race into c46faf2 on GoogleCloudPlatform:pubsub-hp.

*/
ListenableFuture<AckReply> receiveMessage(PubsubMessage message);
void receiveMessage(PubsubMessage message, SettableFuture<AckReply> response);

This comment was marked as spam.

This comment was marked as spam.

@pongad
Copy link
Contributor Author

pongad commented Feb 1, 2017

@garrettjonesgoogle is right that adding final in interface signature doesn't actually do anything, but it could serve as a reminder for users to be careful since we're dealing with concurrent code.
@davidtorres PTAL

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 83.085% when pulling 669d449 on pongad:fix-race into c46faf2 on GoogleCloudPlatform:pubsub-hp.

@pongad pongad merged commit d325afb into googleapis:pubsub-hp Feb 2, 2017
@pongad pongad deleted the fix-race branch February 2, 2017 01:42
Copy link

@davidtorres davidtorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants