Update FakeStream::{trailers,headers} to address thread safety warnings

Returning reference trips thread safety analysis in clang-18, because
returning a reference to a lock protected member is in general
incorrect.

I don't think that this is in practice causing any problems because
tests call one of the waitForX functions before calling trailers/headers
methods and thus by the time trailes/headers is being called synchronization
would have already happened (and in the same thread).

Still, we do want to migrate to a newer toolchain, so we should address
those warnings to make clang-18 and transitively Envoy CI happy.

I'm changing trailers to return a copy of a shared pointer instead of
returning a reference  to a pointer. While it changes the return type,
it should not affect any callers.

For headers it's a bit more complicated, as it returns a reference. To
avoid changing the interface and updating multiple tests that use the
headers method we went with a somewhat different approach that relies on
a couple of assumptions:

1. headers() method is only called from a single thread, which may or
   may not be different from the thread that calls decodedHeaders()
2. headers cannot be updated in place - only replaced completely.

With those two assumptions we can maintain two header maps, one for
internal use (headers_) and one for the clients (client_headers_).

The client_headers_ map is only accessed in the headers() method and
therefore does not require a lock. headers_ map can be accessed
from multiple threads, so it does require a lock, but it's purely
internal and we do not expose it outside of the class.

In the headers() method call we move the data from headers_ to
client_headers_ if the data changed and return a reference to the new
client_headers_.

Signed-off-by: Mikhail Krinkin <[email protected]>
Signed-off-by: Mikhail Krinkin <[email protected]>

Author: krinkinmu

Diff for: envoy/http/header_map.h

@@ -744,6 +744,8 @@ class RequestTrailerMap
     : public HeaderMap,
       public CustomInlineHeaderBase<CustomInlineHeaderRegistry::Type::RequestTrailers> {};
 using RequestTrailerMapPtr = std::unique_ptr<RequestTrailerMap>;
+using RequestTrailerMapSharedPtr = std::shared_ptr<RequestTrailerMap>;
+using RequestTrailerMapConstSharedPtr = std::shared_ptr<const RequestTrailerMap>;
 using RequestTrailerMapOptRef = OptRef<RequestTrailerMap>;
 using RequestTrailerMapOptConstRef = OptRef<const RequestTrailerMap>;
 

Diff for: test/integration/fake_upstream.h

@@ -104,10 +104,17 @@ class FakeStream : public Http::RequestDecoder,
   void readDisable(bool disable);
   const Http::RequestHeaderMap& headers() {
     absl::MutexLock lock(&lock_);
-    return *headers_;
+    if (client_headers_ != headers_) {
+      client_headers_ = headers_;
+    }
+    return *client_headers_;
   }
   void setAddServedByHeader(bool add_header) { add_served_by_header_ = add_header; }
-  const Http::RequestTrailerMapPtr& trailers() { return trailers_; }
+  Http::RequestTrailerMapConstSharedPtr trailers() {
+    absl::MutexLock lock(&lock_);
+    Http::RequestTrailerMapConstSharedPtr trailers{trailers_};
+    return trailers;
+  }
   bool receivedData() { return received_data_; }
   Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() {
     return encoder_.http1StreamEncoderOptions();
@@ -256,13 +263,31 @@ class FakeStream : public Http::RequestDecoder,
 
 protected:
   absl::Mutex lock_;
+  // headers get updated in decodeHeaders() and accessed in headers() methods. Those methods can
+  // be called from different threads, but we can rely on a few assumptions here:
+  //
+  // 1. headers() method is only called from a single thread, which may or may not be different
+  //    from the thread that calls decodeHeaders
+  // 2. headers can only be replaced completely - they are never modified in place.
+  //
+  // With those two assumptions, we can use a synchronization pattern with two header maps, that
+  // allows us to return an unprotected reference to the clients from the headers() method. The
+  // reason why we want to return an unprotected reference is because historically that's what the
+  // interface of the FakeStream was, and over time we accumulated quite a few tests that rely on
+  // that.
+  //
+  // With that in mind we made a decision to preserve the interface and avoid migrating multiple
+  // tests to a new interface when we addressed Clang thread sanitizer warnings.
+  //
+  // That's why we have both headers_ and client_headers_ map below.
   Http::RequestHeaderMapSharedPtr headers_ ABSL_GUARDED_BY(lock_);
   Buffer::OwnedImpl body_ ABSL_GUARDED_BY(lock_);
   FakeHttpConnection& parent_;
 
 private:
   Http::ResponseEncoder& encoder_;
-  Http::RequestTrailerMapPtr trailers_ ABSL_GUARDED_BY(lock_);
+  Http::RequestHeaderMapSharedPtr client_headers_;
+  Http::RequestTrailerMapSharedPtr trailers_ ABSL_GUARDED_BY(lock_);
   bool end_stream_ ABSL_GUARDED_BY(lock_){};
   bool saw_reset_ ABSL_GUARDED_BY(lock_){};
   Grpc::Decoder grpc_decoder_;