Skip to content
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

Fix retry race condition that can lead to double decrementing inFlightSubStreams and so miss calling closed #11026

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

larry-safran
Copy link
Contributor

@larry-safran larry-safran commented Mar 21, 2024

Future.cancel can return true when the executable has started, but the future value hasn't yet been set which was causing us to do an extra decrement. That is what was causing DriveSimControllerRuleTest to fail with the first attempt to fix the retry deadlock.

@larry-safran larry-safran requested a review from temawi March 21, 2024 01:11
@larry-safran larry-safran added the TODO:backport PR needs to be backported. Removed after backport complete label Mar 21, 2024
@larry-safran larry-safran added this to the 1.63 milestone Mar 21, 2024
Copy link
Contributor

@temawi temawi left a comment

Choose a reason for hiding this comment

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

Are these changes covered by an existing unit test or do we need more test code?

… to happen after safeCloseMasterListener was called. Also update a couple of tests to pick up more issues.
final Future<?> retryFuture;
if (scheduledRetry != null) {
// TODO(b/145386688): This access should be guarded by 'this.scheduledRetry.lock'; instead
Copy link
Member

Choose a reason for hiding this comment

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

Why remove these? These look disabled by the @SuppressWarnings("GuardedBy")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way that the comments were done was intrusive and ugly. Changed them all to be on the SuppressWarnings annotation.

Copy link
Contributor

@temawi temawi left a comment

Choose a reason for hiding this comment

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

This seems ok to me, but there is a lot going on in this class and I'm not that familiar with it. I think we would want the @ejona86 seal of approval here as well.

@larry-safran larry-safran merged commit bdb6230 into grpc:master Mar 22, 2024
14 of 15 checks passed
@larry-safran larry-safran deleted the retry_2 branch March 22, 2024 17:02
larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Mar 22, 2024
larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Mar 22, 2024
larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Mar 22, 2024
larry-safran added a commit that referenced this pull request Mar 22, 2024
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

LGTM

larry-safran added a commit that referenced this pull request Mar 22, 2024
larry-safran added a commit that referenced this pull request Mar 26, 2024
@larry-safran larry-safran removed the TODO:backport PR needs to be backported. Removed after backport complete label Mar 27, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants