Skip to content

Commit 91cfa97

Browse files
committed
netty: Avoid volatile attributes on client-side
6efa9ee added `volatile` to `attributes` after TSAN detected a data race that was added in 91d15ce. 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 ... ```
1 parent c346b41 commit 91cfa97

File tree

2 files changed

+11
-5
lines changed

2 files changed

+11
-5
lines changed

netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,19 @@ public ClientTransportLifecycleManager(ManagedClientTransport.Listener listener)
3737
this.listener = listener;
3838
}
3939

40-
public Attributes notifyReady(Attributes attributes) {
40+
public Attributes filterAttributes(Attributes attributes) {
4141
if (transportReady || transportShutdown) {
4242
return attributes;
4343
}
44+
return listener.filterTransport(attributes);
45+
}
46+
47+
public void notifyReady() {
48+
if (transportReady || transportShutdown) {
49+
return;
50+
}
4451
transportReady = true;
45-
attributes = listener.filterTransport(attributes);
4652
listener.transportReady();
47-
return attributes;
4853
}
4954

5055
/**

netty/src/main/java/io/grpc/netty/NettyClientHandler.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ protected void handleNotInUse() {
131131

132132
private WriteQueue clientWriteQueue;
133133
private Http2Ping ping;
134-
private volatile Attributes attributes;
134+
private Attributes attributes;
135135
private InternalChannelz.Security securityInfo;
136136
private Status abruptGoAwayStatus;
137137
private Status channelInactiveReason;
@@ -917,7 +917,8 @@ private class FrameListener extends Http2FrameAdapter {
917917
public void onSettingsRead(ChannelHandlerContext ctx, Http2Settings settings) {
918918
if (firstSettings) {
919919
firstSettings = false;
920-
attributes = lifecycleManager.notifyReady(attributes);
920+
attributes = lifecycleManager.filterAttributes(attributes);
921+
lifecycleManager.notifyReady();
921922
}
922923
}
923924

0 commit comments

Comments
 (0)