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: Make sure outstanding RPCs count in ChannelPool can not go negative #2185

Merged
merged 9 commits into from
Oct 19, 2023

Conversation

blakeli0
Copy link
Collaborator

@blakeli0 blakeli0 commented Oct 19, 2023

fixes: #2081

Add two flags wasClosed and wasReleased to ReleasingClientCall to check various scenarios. The combination of these two flags can make sure the count of outstanding RPCs can never go negative, and help us identify what exactly goes wrong next time it happens.

More background for the purpose of outstandingRPCs

The primary purpose of keeping a count for outstanding RPCs is to track when a channel is
safe to close. In grpc, initialization & starting of rpcs is split between 2 methods:
Channel#newCall() and ClientCall#start. gRPC already has a mechanism to safely close channels
that have rpcs that have been started. However, it does not protect calls that have been
created but not started. In the sequence: Channel#newCall() Channel#shutdown()
ClientCall#Start(), gRpc will error out the call telling the caller that the channel is
shutdown.
Hence, the increment of outstanding RPCs has to happen when the ClientCall is initialized,
as part of Channel#newCall(), not after the ClientCall is started. The decrement of
outstanding RPCs has to happen when the ClientCall is closed or the ClientCall failed to
start.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Oct 19, 2023
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

I'm not sure if all of these logs should be warnings, but I think the safeguards make sense.

blakeli0 and others added 5 commits October 19, 2023 16:08
…lPool.java

Co-authored-by: Mike Eltsufin <meltsufin@google.com>
…lPool.java

Co-authored-by: Mike Eltsufin <meltsufin@google.com>
…lPool.java

Co-authored-by: Mike Eltsufin <meltsufin@google.com>
…lPool.java

Co-authored-by: Mike Eltsufin <meltsufin@google.com>
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Oct 19, 2023
@blakeli0 blakeli0 marked this pull request as ready for review October 19, 2023 20:30
@blakeli0 blakeli0 requested a review from a team as a code owner October 19, 2023 20:30
@blakeli0 blakeli0 added the automerge Merge the pull request once unit tests and other checks pass. label Oct 19, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 19, 2023

[gapic-generator-java-root] SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

61.1% 61.1% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@sonarcloud
Copy link

sonarcloud bot commented Oct 19, 2023

[java_showcase_integration_tests] SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

44.4% 44.4% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@blakeli0 blakeli0 merged commit d2d624d into main Oct 19, 2023
33 of 35 checks passed
@blakeli0 blakeli0 deleted the fix-negative-rpc-count-2 branch October 19, 2023 21:39
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The count of outstanding RPCs in ChannelPool could go negative.
3 participants