Github user ash211 commented on a diff in the pull request: https://github.com/apache/spark/pull/19217#discussion_r146493105 --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java --- @@ -232,20 +232,20 @@ public void run() { }; ServerConnection clientConnection = new ServerConnection(client, timeout); Thread clientThread = factory.newThread(clientConnection); - synchronized (timeout) { --- End diff -- we're no longer synchronizing on the `timeout` here, but I didn't see anywhere else synchronizing on it either (including `ServerConnection`). Given it doesn't escape this method, I'm not sure how multiple threads could ever access `timeout` at once, so it makes sense to remove this synchronization
--- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org