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

Waive need for locking when calling FakeStream::{body,headers,trailers} #38167

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krinkinmu
Copy link
Contributor

@krinkinmu krinkinmu commented Jan 23, 2025

Commit Message:

Those return references to internal protected members, so ideally all the callers should acquire a lock before calling those.

However, all the tests that use FakeStream or one of its derivatives cannot really acquire the right lock because it's a protected member of the class.

We got away with this so far for a few reasons:

  1. Clang thread safety annotations didn't detect this problematic pattern in the clang-14 that we are currently using (potentially because those methods actually acquired locks, even though those locks didn't actually protect much).
  2. The locks are really only needed to synchronize all the waitForX methods, accesors methods like body(), headers() and trailers() are called in tests after the appropriate waitForX method was called.

Disabling thread safety annotations for these methods does not actually make anything worse, because the existing implementation aren't thread safe anyways, however here are a few alternatives to disabling those that I considered and rejected at the moment:

  1. Return copies of body, headers and trailers instead of references, create those copies under a lock - that would be the easiest way to let compiler know that the code is fine, but all three methods return abstract classes and currently there is no easy way to copy them (that's not to say, that copying is impossible in principle);
  2. Expose the lock and require all the callers acquire it - this was my first idea of how to fix the issue, but FakeStream (and it's derivatives) is used quite a lot in tests, so this change will get quite invasive.

Because it does not seem like we really need to lock those methods in practice and given that alternatives to disabling thread safety analysis on those are quite invasive, I figured I can just silence the compiler in this case.

Additional Description: Related to #37911 and fixes one of the issues in #38093
Risk Level: Low
Testing: bazel test //test/server/config_validation:config_fuzz_test --config=clang-libc++ (that's how I found the issue in the first place) + all the regular release gating tests.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

+cc @phlax

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38167 was opened by krinkinmu.

see: more, trace.

@krinkinmu krinkinmu force-pushed the fix-thread-safety-checks branch from 27e05ca to d43f17a Compare January 23, 2025 18:13
@krinkinmu krinkinmu changed the title Correct locking in FakeStream Waive need for locking when calling FakeStream::{body,headers,trailers} Jan 23, 2025
Those return references to internal protected members, so ideally all
the callers should acquire a lock before calling those.

However, all the tests that use FakeStream or one of its derivatives
cannot really acquire the right lock because it's a protected member
of the class.

We got away with this so far for a few reasons:

1. Clang thread safety annotations didn't detect this problematic
   pattern in the clang-14 that we are currently using (potentially
   because those methods actually acquired locks, even though those
   locks didn't actually protect much).
2. The locks are really only needed to synchronize all the waitForX
   methods, accesors methods like body(), headers() and trailers() are
   called in tests after the appropriate waitForX method was called.

Disabling thread safety annotations for these methods does not actually
make anything worse, because the existing implementation aren't thread
safe anyways, however here are a few alternatives to disabling those
that I considered and rejected at the moment:

1. Return copies of body, headers and trailers instead of references,
   create those copies under a lock - that would be the easiest way to
   let compiler know that the code is fine, but all three methods return
   abstract classes and currently there is no easy way to copy them
   (that's not to say, that copying is impossible in principle);
2. Expose the lock and require all the callers acquire it - this was my
   first idea of how to fix the issue, but FakeStream (and it's
   derivatives) is used quite a lot in tests, so this change will get
   quite invasive.

Because it does not seem like we really need to lock those methods in
practice and given that alternatives to disabling thread safety analysis
on those are quite invasive, I figured I can just silence the compiler
in this case.

Signed-off-by: Mikhail Krinkin <[email protected]>
@krinkinmu krinkinmu force-pushed the fix-thread-safety-checks branch from d43f17a to 55c6947 Compare January 23, 2025 18:26
@krinkinmu
Copy link
Contributor Author

/retest flaky test

@krinkinmu krinkinmu marked this pull request as ready for review January 23, 2025 23:27
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.

1 participant