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: ensure that channel pool ref count never goes negative (take2) #2065

Closed

Conversation

igorbernstein2
Copy link
Contributor

I'm not sure the root cause that might cause this, but this guard should tone down the noise that is created and help us identify the root cause

Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

I'm not sure the root cause that might cause this, but this guard should tone down the noise that is created and help us identify the root cause
@igorbernstein2 igorbernstein2 requested a review from a team as a code owner October 4, 2023 18:12
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Oct 4, 2023
@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 4, 2023

void release() {
if (!wasReleased.compareAndSet(false, true)) {
Exception e = new IllegalStateException("Entry was already released");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is another stacktrace we don't want to log, can we just log the WARNING without a stacktrace?
Or if we think this is a true error, then we can throw the Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely need a stacktrace here, it will tell us how the refcount got negative. Otherwise we are no wiser than before.....we are just hiding the problem under the rug. And in this case alert firing woud be correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

To me Entry was already released is a true error, why are we just logging instead of throwing an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because throwing will bubble up to the enduser, who had nothing to do with the error. Instead I want to keep it localized to the double release and protect the rest of the application

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 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 0 Code Smells

78.9% 78.9% 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 6, 2023

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@meltsufin
Copy link
Member

@igorbernstein2 @blakeli0 Can you please create a corresponding issue for better tracking?

@blakeli0
Copy link
Collaborator

blakeli0 commented Oct 9, 2023

@igorbernstein2 @blakeli0 Can you please create a corresponding issue for better tracking?

Created #2081 to track it.

@blakeli0
Copy link
Collaborator

Closing in favor of #2185

@blakeli0 blakeli0 closed this Oct 20, 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.

3 participants