tsan, xds: fix data races in ServerWrapperForXds #8114
Conversation
| @@ -290,7 +292,7 @@ private static boolean isPermanentErrorFromXds(Throwable throwable) { | |||
|
|
|||
| private void cleanupStartRetryTaskAndShutdownDelegateAndXdsClient(boolean shutdownNow) { | |||
| Server delegateCopy = null; | |||
| synchronized (this) { | |||
| synchronized (ServerWrapperForXds.this) { | |||
There was a problem hiding this comment.
this in this context only means ServerWrapperForXds.this so what does this change achieve? To make Tsan happy?
There was a problem hiding this comment.
I have a feeling that with the fix on line 242 (synchronizing on ServerWrapperForXds.this instead of ServerWrapperForXds.StartRetryTask.this) was all that was needed and the warning for this line should disappear with this unneeded change
There was a problem hiding this comment.
I have different understanding. It looks the original this means StartRetryTask.this instead of ServerWrapperForXds.this because this is in nested class.
And L242 looks a different usage of the racy object startRetryTask, so it needs synchronization on the ServerWrapperForXds.this, not StartRetryTask.this, which the original synchronization method locks on.
There was a problem hiding this comment.
cleanupStartRetryTaskAndShutdownDelegateAndXdsClient is a method of the outer class ServerWrapperForXds and not of a nested class. So ServerWrapperForXds. is not necessary. I was just wondering about Tsan warning for this line.
There was a problem hiding this comment.
You are right it is outside of the nested class, then ServerWrapperForXds.this change is not needed. Not a tsan problem, Tsan was reporting data races in all those places and yes the l242 is the problem and it is passing now w/o the change here. Thanks for pointing out.
fix tsan complain