Update comments to clarify the approach with two maps better

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

Author: krinkinmu

test/integration/fake_upstream.h

@@ -263,21 +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:
+  // 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:
   //
   // 1. headers() method is only called from a single thread, which may or may not be different
-  //    from the thread that calls decodeHeaders
+  //    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.
+  // approach works as follows:
   //
-  // 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.
+  // 1. When headers are updated in decodeHeaders(), we store them in the internal headers_ map
+  // 2. When headers() is called, we check if the internal headers_ and external client_headers_
+  //    map point to different places, and if so, we update client_headers_ map to point to the
+  //    same map as internal headers_.
+  //
+  // Because both internal headers_ and client_headers_ are shared pointers, the map will stay
+  // around as long as at least one of those pointers keeps a reference to the map and thus
+  // the returned reference stay valid as well. And because headers() is only called from a single
+  // thread, it's safe to return a reference to client_headers_ from headers() call.
+  //
+  // The reason why we want to return an unprotected reference is because historically that's what
+  // the interface of the FakeStream::headers() was, and over time we accumulated quite a few tests
+  // that rely on that. Changing the interface would require updating all the tests and we decided
+  // not to do that.
   //
   // That's why we have both headers_ and client_headers_ map below.
   Http::RequestHeaderMapSharedPtr headers_ ABSL_GUARDED_BY(lock_);
@@ -286,6 +296,8 @@ class FakeStream : public Http::RequestDecoder,
 
 private:
   Http::ResponseEncoder& encoder_;
+  // See the comment for headers_ above that explains why we need both headers_ and
+  // client_headers_.
   Http::RequestHeaderMapSharedPtr client_headers_;
   Http::RequestTrailerMapSharedPtr trailers_ ABSL_GUARDED_BY(lock_);
   bool end_stream_ ABSL_GUARDED_BY(lock_){};