Skip to content

Commit

Permalink
Wave need for locking when calling FakeStrea::{body,headers,trailers}
Browse files Browse the repository at this point in the history
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 <mkrinkin@microsoft.com>
  • Loading branch information
krinkinmu committed Jan 23, 2025
1 parent 41d8ccc commit 55c6947
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 9 deletions.
1 change: 1 addition & 0 deletions test/integration/fake_upstream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ AssertionResult FakeStream::waitForData(Event::Dispatcher& client_dispatcher,
absl::string_view data, milliseconds timeout) {
auto succeeded = waitForData(client_dispatcher, data.length(), timeout);
if (succeeded) {
absl::MutexLock lock(&lock_);
Buffer::OwnedImpl buffer(data.data(), data.length());
if (!TestUtility::buffersEqual(body(), buffer)) {
return AssertionFailure() << body().toString() << " not equal to " << data;
Expand Down
12 changes: 3 additions & 9 deletions test/integration/fake_upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,7 @@ class FakeStream : public Http::RequestDecoder,
absl::MutexLock lock(&lock_);
return body_.length();
}
Buffer::Instance& body() {
absl::MutexLock lock(&lock_);
return body_;
}
Buffer::Instance& body() ABSL_NO_THREAD_SAFETY_ANALYSIS { return body_; }
bool complete() {
absl::MutexLock lock(&lock_);
return end_stream_;
Expand All @@ -97,12 +94,9 @@ class FakeStream : public Http::RequestDecoder,
void encodeResetStream();
void encodeMetadata(const Http::MetadataMapVector& metadata_map_vector);
void readDisable(bool disable);
const Http::RequestHeaderMap& headers() {
absl::MutexLock lock(&lock_);
return *headers_;
}
const Http::RequestHeaderMap& headers() ABSL_NO_THREAD_SAFETY_ANALYSIS { return *headers_; }
void setAddServedByHeader(bool add_header) { add_served_by_header_ = add_header; }
const Http::RequestTrailerMapPtr& trailers() { return trailers_; }
const Http::RequestTrailerMapPtr& trailers() ABSL_NO_THREAD_SAFETY_ANALYSIS { return trailers_; }
bool receivedData() { return received_data_; }
Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() {
return encoder_.http1StreamEncoderOptions();
Expand Down

0 comments on commit 55c6947

Please sign in to comment.