Skip to content

Conversation

kannanjgithub
Copy link
Contributor

No description provided.

Copy link
Member

@shivaspeaks shivaspeaks left a comment

Choose a reason for hiding this comment

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

I believe we had an option of making the method synchronized but since this is a test-only method (@VisibleForTesting), we preferred just suppressing it.

Though can we add a comment here saying something like in the added commit?

Revert comment since we don't have usage currently of this accessor and it seems the test code will be a reader and doesn't have to lock.
@kannanjgithub kannanjgithub merged commit ead6a54 into grpc:master Sep 16, 2025
22 of 23 checks passed
@ejona86
Copy link
Member

ejona86 commented Sep 16, 2025

Why would we prefer to suppress the warning? If it is test-only then there's no cost to it being synchronized...

If it were a production method it'd have been better to use @GuardedBy("this") to propagate the requirement to the caller. Basically, I don't think we should be in the habit of suppressing synchronization warnings. The only case to suppress is when the checker isn't able to verify something and it is too hard to change it to be verified (e.g., in okhttp some of the lock usage can't be verified by the tool; we'd still love to verify that though).

@ejona86
Copy link
Member

ejona86 commented Sep 17, 2025

Created #12364 to fix the synchronization

@kannanjgithub
Copy link
Contributor Author

Got it, thanks.

AgraVator pushed a commit to AgraVator/grpc-java that referenced this pull request Sep 26, 2025
Co-authored-by: MV Shiva <speakupshiva@gmail.com>
AgraVator pushed a commit to AgraVator/grpc-java that referenced this pull request Sep 26, 2025
Co-authored-by: MV Shiva <speakupshiva@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants