From 91cfa97c284ea31be73d92d8b03f8e9af8c18eb6 Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Thu, 11 Jan 2024 15:31:27 -0800
Subject: [PATCH] netty: Avoid volatile attributes on client-side

6efa9ee3 added `volatile` to `attributes` after TSAN detected a data
race that was added in 91d15ce4. The race was because attributes may be
read from another thread after `transportReady()`, and the
post-filtering assignment occurred after `transportReady()`. The code
now filters the attributes separately so they are updated before calling
`transportReady()`.

Original TSAN failure:
```
  Read of size 4 at 0x0000cd44769c by thread T23:
    #0 io.grpc.netty.NettyClientHandler.getAttributes()Lio/grpc/Attributes; NettyClientHandler.java:327
    #1 io.grpc.netty.NettyClientTransport.getAttributes()Lio/grpc/Attributes; NettyClientTransport.java:363
    #2 io.grpc.netty.NettyClientTransport.newStream(Lio/grpc/MethodDescriptor;Lio/grpc/Metadata;Lio/grpc/CallOptions;[Lio/grpc/ClientStreamTracer;)Lio/grpc/internal/ClientStream; NettyClientTransport.java:183
    #3 io.grpc.internal.MetadataApplierImpl.apply(Lio/grpc/Metadata;)V MetadataApplierImpl.java:74
    #4 io.grpc.auth.GoogleAuthLibraryCallCredentials$1.onSuccess(Ljava/util/Map;)V GoogleAuthLibraryCallCredentials.java:141
    #5 com.google.auth.oauth2.OAuth2Credentials$FutureCallbackToMetadataCallbackAdapter.onSuccess(Lcom/google/auth/oauth2/OAuth2Credentials$OAuthValue;)V OAuth2Credentials.java:534
    #6 com.google.auth.oauth2.OAuth2Credentials$FutureCallbackToMetadataCallbackAdapter.onSuccess(Ljava/lang/Object;)V OAuth2Credentials.java:525
    ...

  Previous write of size 4 at 0x0000cd44769c by thread T24:
    #0 io.grpc.netty.NettyClientHandler$FrameListener.onSettingsRead(Lio/netty/channel/ChannelHandlerContext;Lio/netty/handler/codec/http2/Http2Settings;)V NettyClientHandler.java:920
    #1 io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onSettingsRead(Lio/netty/channel/ChannelHandlerContext;Lio/netty/handler/codec/http2/Http2Settings;)V DefaultHttp2ConnectionDecoder.java:515
    ...
```
---
 .../grpc/netty/ClientTransportLifecycleManager.java   | 11 ++++++++---
 .../main/java/io/grpc/netty/NettyClientHandler.java   |  5 +++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java b/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java
index 4addf59215..34f72ab97b 100644
--- a/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java
+++ b/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java
@@ -37,14 +37,19 @@ final class ClientTransportLifecycleManager {
     this.listener = listener;
   }
 
-  public Attributes notifyReady(Attributes attributes) {
+  public Attributes filterAttributes(Attributes attributes) {
     if (transportReady || transportShutdown) {
       return attributes;
     }
+    return listener.filterTransport(attributes);
+  }
+
+  public void notifyReady() {
+    if (transportReady || transportShutdown) {
+      return;
+    }
     transportReady = true;
-    attributes = listener.filterTransport(attributes);
     listener.transportReady();
-    return attributes;
   }
 
   /**
diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java
index 3c8c128d15..eb4dbf8cc6 100644
--- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java
+++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java
@@ -131,7 +131,7 @@ class NettyClientHandler extends AbstractNettyHandler {
 
   private WriteQueue clientWriteQueue;
   private Http2Ping ping;
-  private volatile Attributes attributes;
+  private Attributes attributes;
   private InternalChannelz.Security securityInfo;
   private Status abruptGoAwayStatus;
   private Status channelInactiveReason;
@@ -917,7 +917,8 @@ class NettyClientHandler extends AbstractNettyHandler {
     public void onSettingsRead(ChannelHandlerContext ctx, Http2Settings settings) {
       if (firstSettings) {
         firstSettings = false;
-        attributes = lifecycleManager.notifyReady(attributes);
+        attributes = lifecycleManager.filterAttributes(attributes);
+        lifecycleManager.notifyReady();
       }
     }