Skip to content

Commit

Permalink
netty: Avoid volatile attributes on client-side
Browse files Browse the repository at this point in the history
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
    grpc#3 io.grpc.internal.MetadataApplierImpl.apply(Lio/grpc/Metadata;)V MetadataApplierImpl.java:74
    grpc#4 io.grpc.auth.GoogleAuthLibraryCallCredentials$1.onSuccess(Ljava/util/Map;)V GoogleAuthLibraryCallCredentials.java:141
    grpc#5 com.google.auth.oauth2.OAuth2Credentials$FutureCallbackToMetadataCallbackAdapter.onSuccess(Lcom/google/auth/oauth2/OAuth2Credentials$OAuthValue;)V OAuth2Credentials.java:534
    grpc#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
    ...
```
  • Loading branch information
ejona86 committed Jan 12, 2024
1 parent c346b41 commit 91cfa97
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,19 @@ public ClientTransportLifecycleManager(ManagedClientTransport.Listener listener)
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;
}

/**
Expand Down
5 changes: 3 additions & 2 deletions netty/src/main/java/io/grpc/netty/NettyClientHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ protected void handleNotInUse() {

private WriteQueue clientWriteQueue;
private Http2Ping ping;
private volatile Attributes attributes;
private Attributes attributes;
private InternalChannelz.Security securityInfo;
private Status abruptGoAwayStatus;
private Status channelInactiveReason;
Expand Down Expand Up @@ -917,7 +917,8 @@ private class FrameListener extends Http2FrameAdapter {
public void onSettingsRead(ChannelHandlerContext ctx, Http2Settings settings) {
if (firstSettings) {
firstSettings = false;
attributes = lifecycleManager.notifyReady(attributes);
attributes = lifecycleManager.filterAttributes(attributes);
lifecycleManager.notifyReady();
}
}

Expand Down

0 comments on commit 91cfa97

Please sign in to comment.