Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Race on NettyClientTransport.start #2015

Closed
carl-mastrangelo opened this issue Jul 2, 2016 · 7 comments
Closed

Race on NettyClientTransport.start #2015

carl-mastrangelo opened this issue Jul 2, 2016 · 7 comments
Assignees
@buchgr
Milestone
1.0

Comments

@carl-mastrangelo
Copy link
Contributor

Bootstrap.connect seems to add a listener to a ChannelFuture that is concurrently modified by the NioEventLoop and the client provided executor in ConcurrencyTest

Conflicting accesses:

WARNING: ThreadSanitizer: data race (pid=734706)
  Write of size 8 at 0x7f4e49d10538 by thread T86 (mutexes: write M70410, write M82043):
    #0 io.netty.util.concurrent.DefaultPromise.addListener0(Lio/netty/util/concurrent/GenericFutureListener;)V (DefaultPromise.java:526)  
    #1 io.netty.util.concurrent.DefaultPromise.addListener(Lio/netty/util/concurrent/GenericFutureListener;)Lio/netty/util/concurrent/Promise; (DefaultPromise.java:163)  
    #2 io.netty.channel.DefaultChannelPromise.addListener(Lio/netty/util/concurrent/GenericFutureListener;)Lio/netty/channel/ChannelPromise; (DefaultChannelPromise.java:93)  
    #3 io.netty.channel.DefaultChannelPromise.addListener(Lio/netty/util/concurrent/GenericFutureListener;)Lio/netty/channel/ChannelFuture; (DefaultChannelPromise.java:28)  
    #4 io.netty.bootstrap.Bootstrap.doResolveAndConnect(Ljava/net/SocketAddress;Ljava/net/SocketAddress;)Lio/netty/channel/ChannelFuture; (Bootstrap.java:173)  
    #5 io.netty.bootstrap.Bootstrap.connect(Ljava/net/SocketAddress;)Lio/netty/channel/ChannelFuture; (Bootstrap.java:144)  
    #6 io.grpc.netty.NettyClientTransport.start(Lio/grpc/internal/ManagedClientTransport$Listener;)V (NettyClientTransport.java:156)  
    #7 io.grpc.internal.ForwardingConnectionClientTransport.start(Lio/grpc/internal/ManagedClientTransport$Listener;)V (ForwardingConnectionClientTransport.java:45)  
    #8 io.grpc.internal.TransportSet.startNewTransport(Lio/grpc/internal/DelayedClientTransport;)V (TransportSet.java:206)  
    #9 io.grpc.internal.TransportSet.obtainActiveTransport()Lio/grpc/internal/ClientTransport; (TransportSet.java:179)  
    #10 io.grpc.internal.ManagedChannelImpl$3.getTransport(Lio/grpc/EquivalentAddressGroup;)Lio/grpc/internal/ClientTransport; (ManagedChannelImpl.java:476)  
    #11 io.grpc.internal.ManagedChannelImpl$3.getTransport(Lio/grpc/EquivalentAddressGroup;)Ljava/lang/Object; (ManagedChannelImpl.java:432)  
    #12 io.grpc.DummyLoadBalancerFactory$DummyLoadBalancer.pickTransport(Lio/grpc/Attributes;)Ljava/lang/Object; (DummyLoadBalancerFactory.java:105)  
    #13 io.grpc.internal.ManagedChannelImpl$1.get(Lio/grpc/CallOptions;)Lio/grpc/internal/ClientTransport; (ManagedChannelImpl.java:149)  
    #14 io.grpc.internal.ClientCallImpl.start(Lio/grpc/ClientCall$Listener;Lio/grpc/Metadata;)V (ClientCallImpl.java:201)  
    #15 io.grpc.stub.ClientCalls.startCall(Lio/grpc/ClientCall;Lio/grpc/ClientCall$Listener;Z)V (ClientCalls.java:248)  
    #16 io.grpc.stub.ClientCalls.asyncUnaryRequestCall(Lio/grpc/ClientCall;Ljava/lang/Object;Lio/grpc/ClientCall$Listener;Z)V (ClientCalls.java:227)  
    #17 io.grpc.stub.ClientCalls.asyncUnaryRequestCall(Lio/grpc/ClientCall;Ljava/lang/Object;Lio/grpc/stub/StreamObserver;Z)V (ClientCalls.java:215)  
    #18 io.grpc.stub.ClientCalls.asyncServerStreamingCall(Lio/grpc/ClientCall;Ljava/lang/Object;Lio/grpc/stub/StreamObserver;)V (ClientCalls.java:87)  
    #19 io.grpc.testing.integration.TestServiceGrpc$TestServiceStub.streamingOutputCall(Lio/grpc/testing/integration/Messages$StreamingOutputCallRequest;Lio/grpc/stub/StreamObserver;)V (TestServiceGrpc.java:311)  
    #20 io.grpc.testing.integration.ConcurrencyTest$ClientWorker.run()V (ConcurrencyTest.java:138)  
    #21 java.util.concurrent.ThreadPoolExecutor.runWorker(Ljava/util/concurrent/ThreadPoolExecutor$Worker;)V (ThreadPoolExecutor.java:1142)  
    #22 java.util.concurrent.ThreadPoolExecutor$Worker.run()V (ThreadPoolExecutor.java:617)  
    #23 java.lang.Thread.run()V (Thread.java:745)  
    #24 (Generated Stub)  

  Previous read of size 8 at 0x7f4e49d10538 by thread T142:
    #0 io.netty.util.concurrent.DefaultPromise.notifyListeners()V (DefaultPromise.java:417)  
    #1 io.netty.util.concurrent.DefaultPromise.trySuccess(Ljava/lang/Object;)Z (DefaultPromise.java:108)  
    #2 io.netty.channel.DefaultChannelPromise.trySuccess()Z (DefaultChannelPromise.java:82)  
    #3 io.netty.channel.AbstractChannel$AbstractUnsafe.safeSetSuccess(Lio/netty/channel/ChannelPromise;)V (AbstractChannel.java:884)  
    #4 io.netty.channel.AbstractChannel$AbstractUnsafe.register0(Lio/netty/channel/ChannelPromise;)V (AbstractChannel.java:497)  
    #5 io.netty.channel.AbstractChannel$AbstractUnsafe.access$200(Lio/netty/channel/AbstractChannel$AbstractUnsafe;Lio/netty/channel/ChannelPromise;)V (AbstractChannel.java:412)  
    #6 io.netty.channel.AbstractChannel$AbstractUnsafe$1.run()V (AbstractChannel.java:471)  
    #7 io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(J)Z (SingleThreadEventExecutor.java:339)  
    #8 io.netty.channel.nio.NioEventLoop.run()V (NioEventLoop.java:393)  
    #9 io.netty.util.concurrent.SingleThreadEventExecutor$5.run()V (SingleThreadEventExecutor.java:742)  
    #10 io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run()V (DefaultThreadFactory.java:145)  
    #11 java.lang.Thread.run()V (Thread.java:745)  
    #12 (Generated Stub)
@carl-mastrangelo carl-mastrangelo added this to the 1.0 milestone Jul 2, 2016
@buchgr buchgr mentioned this issue Jul 2, 2016
Closed
@buchgr
Copy link
Collaborator

buchgr commented Jul 2, 2016

Looks like a race in DefaultPromise. I opened an issue on Netty issue tracker netty/netty#5489

@buchgr buchgr self-assigned this Jul 2, 2016
@buchgr buchgr mentioned this issue Jul 4, 2016
Closed
buchgr added a commit to buchgr/netty that referenced this issue Jul 4, 2016
@buchgr
Fix improper synchronization in DefaultPromise. Fixes netty#5489
ad716af 
Motivation:

A race detector found that DefaultPromise.listeners is improperly synchronized [1].
Worst case a listener will not be executed when the promise is completed.

Modifications:

Make DefaultPromise.listeners a volatile.

Result:

Hopefully, DefaultPromise is more correct under concurrent execution.

[1] grpc/grpc-java#2015
normanmaurer pushed a commit to netty/netty that referenced this issue Jul 4, 2016
@buchgr @normanmaurer
Fix improper synchronization in DefaultPromise. Fixes #5489
b0a5d4c 
Motivation:

A race detector found that DefaultPromise.listeners is improperly synchronized [1].
Worst case a listener will not be executed when the promise is completed.

Modifications:

Make DefaultPromise.listeners a volatile.

Result:

Hopefully, DefaultPromise is more correct under concurrent execution.

[1] grpc/grpc-java#2015
normanmaurer pushed a commit to netty/netty that referenced this issue Jul 4, 2016
@buchgr @normanmaurer
Fix improper synchronization in DefaultPromise. Fixes #5489
88dff2a 
Motivation:

A race detector found that DefaultPromise.listeners is improperly synchronized [1].
Worst case a listener will not be executed when the promise is completed.

Modifications:

Make DefaultPromise.listeners a volatile.

Result:

Hopefully, DefaultPromise is more correct under concurrent execution.

[1] grpc/grpc-java#2015
@buchgr
Copy link
Collaborator

buchgr commented Jul 4, 2016

Should be fixed once we upgrade to Netty 4.1.3.Final (will be released end of week).

@carl-mastrangelo
Copy link
Contributor Author

@buchgr thanks! I can test this once it has been rolled out into a named release. It's currently rather difficult to pull in unreleased builds.

@normanmaurer
Copy link

@carl-mastrangelo can't you just pull in a snapshot ?

@carl-mastrangelo
Copy link
Contributor Author

@normanmaurer Surprisingly no. its somewhat of a manual endeavor, hampered by the fact that Google's version of Java is different enough from the OpenJDK to make the process not straight forward. Prefabbed jars are okay though (which is why we typically wait for full releases).

@normanmaurer
Copy link

@carl-mastrangelo bummer... I hoped you could just grab a snapshot jar from sonatype and go with it:

https://oss.sonatype.org/content/repositories/snapshots/io/netty/

@buchgr buchgr mentioned this issue Jul 15, 2016
Merged
@buchgr
Copy link
Collaborator

buchgr commented Jul 18, 2016

fixed by 0ad5948

@buchgr buchgr closed this as completed Jul 18, 2016
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this issue Sep 10, 2017
@buchgr
Fix improper synchronization in DefaultPromise. Fixes netty#5489
e1b0dc9 
Motivation:

A race detector found that DefaultPromise.listeners is improperly synchronized [1].
Worst case a listener will not be executed when the promise is completed.

Modifications:

Make DefaultPromise.listeners a volatile.

Result:

Hopefully, DefaultPromise is more correct under concurrent execution.

[1] grpc/grpc-java#2015
@lock lock bot locked as resolved and limited conversation to collaborators Sep 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@buchgr buchgr

Labels
None yet
Projects
None yet
Milestone
1.0
Development

No branches or pull requests
3 participants
@normanmaurer @buchgr @carl-mastrangelo