[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
GitHub user vanzin opened a pull request: https://github.com/apache/spark/pull/20223 [SPARK-23020][core] Fix races in launcher code, test. The race in the code is because the handle might update its state to the wrong state if the connection handling thread is still processing incoming data; so the handle needs to wait for the connection to finish up before checking the final state. The race in the test is because when waiting for a handle to reach a final state, the waitFor() method needs to wait until all handle state is updated (which also includes waiting for the connection thread above to finish). Otherwise, waitFor() may return too early, which would cause a bunch of different races (like the listener not being yet notified of the state change, or being in the middle of being notified, or the handle not being properly disposed and causing postChecks() to assert). On top of that I found, by code inspection, a couple of potential races that could make a handle end up in the wrong state when being killed. Tested by running the existing unit tests a lot (and not seeing the errors I was seeing before). You can merge this pull request into a Git repository by running: $ git pull https://github.com/vanzin/spark SPARK-23020 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/20223.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #20223 commit 5139f605904996667d8d97941172bbe9d366a579 Author: Marcelo Vanzin Date: 2018-01-10T17:56:17Z [SPARK-23020][core] Fix races in launcher code, test. The race in the code is because the handle might update its state to the wrong state if the connection handling thread is still processing incoming data; so the handle needs to wait for the connection to finish up before checking the final state. The race in the test is because when waiting for a handle to reach a final state, the waitFor() method needs to wait until all handle state is updated (which also includes waiting for the connection thread above to finish). Otherwise, waitFor() may return too early, which would cause a bunch of different races (like the listener not being yet notified of the state change, or being in the middle of being notified, or the handle not being properly disposed and causing postChecks() to assert). On top of that I found, by code inspection, a couple of potential races that could make a handle end up in the wrong state when being killed. Tested by running the existing unit tests a lot (and not seeing the errors I was seeing before). --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user cloud-fan commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r160952457 --- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java --- @@ -91,10 +92,15 @@ LauncherConnection getConnection() { return connection; } - boolean isDisposed() { + synchronized boolean isDisposed() { return disposed; --- End diff -- can we simply mark `disposed` as `transient`? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user cloud-fan commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r160956968 --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherConnection.java --- @@ -95,15 +95,15 @@ protected synchronized void send(Message msg) throws IOException { } @Override - public void close() throws IOException { + public synchronized void close() throws IOException { if (!closed) { - synchronized (this) { --- End diff -- what's wrong with this? It's a classic "double-checked locking" in java. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user cloud-fan commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r160957301 --- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java --- @@ -137,7 +139,9 @@ public void testInProcessLauncher() throws Exception { // Here DAGScheduler is stopped, while SparkContext.clearActiveContext may not be called yet. // Wait for a reasonable amount of time to avoid creating two active SparkContext in JVM. --- End diff -- why is `500 ms` not a reasonable waiting time anymore? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r160989352 --- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java --- @@ -91,10 +92,15 @@ LauncherConnection getConnection() { return connection; } - boolean isDisposed() { + synchronized boolean isDisposed() { --- End diff -- why do we need `synchronized` here --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r160991236 --- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java --- @@ -71,15 +71,16 @@ public void stop() { @Override public synchronized void disconnect() { if (!disposed) { --- End diff -- `if(!isDisposed())` ? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r160989105 --- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java --- @@ -91,10 +92,15 @@ LauncherConnection getConnection() { return connection; } - boolean isDisposed() { + synchronized boolean isDisposed() { return disposed; } + synchronized void markDisposed() { --- End diff -- why do we need `synchronized` here? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161006918 --- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java --- @@ -137,7 +139,9 @@ public void testInProcessLauncher() throws Exception { // Here DAGScheduler is stopped, while SparkContext.clearActiveContext may not be called yet. // Wait for a reasonable amount of time to avoid creating two active SparkContext in JVM. --- End diff -- Here Vanzin is waiting for the active SparkContext being empty. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161022567 --- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java --- @@ -91,10 +92,15 @@ LauncherConnection getConnection() { return connection; } - boolean isDisposed() { + synchronized boolean isDisposed() { return disposed; --- End diff -- The `synchronized` is not protecting the variable, but all the actions that happen around the code that sets the variable. So this method returning guarantees that either all the disposal tasks have run or none have. That being said `ServerConnection.close` is not really holding the handle lock as it should, so I might have to make some changes here. These are not really contended locks. In the worst case there will be 2 threads looking the them. So trying to come up with a finer-grained locking scheme here sounds like more trouble than it's worth. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161022671 --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherConnection.java --- @@ -95,15 +95,15 @@ protected synchronized void send(Message msg) throws IOException { } @Override - public void close() throws IOException { + public synchronized void close() throws IOException { if (!closed) { - synchronized (this) { --- End diff -- The sub-class needs coarser-grained locking so this wasn't really doing anything useful. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161022921 --- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java --- @@ -137,7 +139,9 @@ public void testInProcessLauncher() throws Exception { // Here DAGScheduler is stopped, while SparkContext.clearActiveContext may not be called yet. // Wait for a reasonable amount of time to avoid creating two active SparkContext in JVM. --- End diff -- It's not that 500ms is not reasonable, it's that this code will return more quickly when the context shuts down in under 500ms, have a longer grace period in case it ever takes more than 500ms, and fail the test here if the context does not shut down. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161564385 --- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java --- @@ -48,14 +48,16 @@ public synchronized void disconnect() { @Override public synchronized void kill() { -disconnect(); -if (childProc != null) { - if (childProc.isAlive()) { -childProc.destroyForcibly(); +if (!isDisposed()) { + setState(State.KILLED); --- End diff -- Why put `setState` in the front? It should be OK but in previous code some of the `setState` is called in the end. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user cloud-fan commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161651289 --- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java --- @@ -48,14 +48,16 @@ public synchronized void disconnect() { @Override public synchronized void kill() { -disconnect(); -if (childProc != null) { - if (childProc.isAlive()) { -childProc.destroyForcibly(); +if (!isDisposed()) { + setState(State.KILLED); --- End diff -- I have the same question, but should be OK as we always synchronize when touching the state. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/20223 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20223: [SPARK-23020][core] Fix races in launcher code, t...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/20223#discussion_r161834649 --- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java --- @@ -48,14 +48,16 @@ public synchronized void disconnect() { @Override public synchronized void kill() { -disconnect(); -if (childProc != null) { - if (childProc.isAlive()) { -childProc.destroyForcibly(); +if (!isDisposed()) { + setState(State.KILLED); --- End diff -- I changed this because the old disconnect code, at least, might change the handle's state. It's easier to put this call first and not have to reason about whether that will happen. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org