[SPARK-23020][CORE] Fix races in launcher code, test. #20297

vanzin · 2018-01-17T19:44:20Z

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.

The original version of this fix introduced the flipped
version of the first race described above; the connection
closing might override the handle state before the
handle might have a chance to do cleanup. The fix there
is to only dispose of the handle from the connection
when there is an error, and let the handle dispose
itself in the normal case.

The fix also caused a bug in YarnClusterSuite to be surfaced;
the code was checking for a file in the classpath that was
not expected to be there in client mode. Because of the above
issues, the error was not propagating correctly and the (buggy)
test was incorrectly passing.

Tested by running the existing unit tests a lot (and not
seeing the errors I was seeing before).

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. The original version of this fix introduced the flipped version of the first race described above; the connection closing might override the handle state before the handle might have a chance to do cleanup. The fix there is to only dispose of the handle from the connection when there is an error, and let the handle dispose itself in the normal case. The fix also cause a bug in YarnClusterSuite to be surfaced; the code was checking for a file in the classpath that was not expected to be there in client mode. Because of the above issues, the error was not propagating correctly and the (buggy) test was incorrectly passing. Tested by running the existing unit tests a lot (and not seeing the errors I was seeing before).

vanzin · 2018-01-17T19:45:50Z

launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java

@@ -331,23 +358,27 @@ protected void handle(Message msg) throws IOException {
          timeout.cancel();
        }
        close();
+        if (handle != null) {


This is the fix for the new race described in the summary (the code is moved here from below).

This changes behavior slightly: the handle now waits for the child process / thread to finish before disposing itself, whereas before that would happen as soon as the connection with the child process / thread was closed.

vanzin · 2018-01-17T19:46:09Z

resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala

@@ -381,7 +381,9 @@ private object YarnClusterDriver extends Logging with Matchers {

      // Verify that the config archive is correctly placed in the classpath of all containers.
      val confFile = "/" + Client.SPARK_CONF_FILE
-      assert(getClass().getResource(confFile) != null)
+      if (conf.getOption(SparkLauncher.DEPLOY_MODE) == Some("cluster")) {


This is the bug in the YARN tests that the fix uncovered.

vanzin · 2018-01-17T19:46:39Z

@sameeragarwal @cloud-fan @gengliangwang since you guys looked at the previous PR.

SparkQA · 2018-01-17T23:46:09Z

Test build #86289 has finished for PR 20297 at commit 8bde21a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-01-18T18:43:41Z

retest this please

SparkQA · 2018-01-18T22:38:03Z

Test build #86351 has finished for PR 20297 at commit 8bde21a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-01-19T02:26:18Z

launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java

-      if (childProc.isAlive()) {
-        childProc.destroyForcibly();
+    if (!isDisposed()) {
+      setState(State.KILLED);


so we should set the state to KILLED once the kill method is called? Even the code below fails(throw exception), the state should still be KILLED?

+1，I have the same question in last review. We should figure it out.

None of the calls below should raise exceptions. Even the socket close is wrapped in a try..catch.

Even the order doesn't matter, I think it's more conventional to set the state at the end.

Then the comment I made in the previous PR applies. Closing the socket / killing the child can have other implications (like changing the state) and it's easier to reason about what happens if the desired state change happens first.

ok makes sense.

cloud-fan · 2018-01-19T02:27:23Z

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 {


do we still need to change this method?

We never needed to change it, but the extra code wasn't doing anything useful, so I chose the simpler version.

cloud-fan · 2018-01-19T02:31:21Z

launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java

-          LOG.log(Level.WARNING, "Lost connection to spark application.");
-          handle.setState(SparkAppHandle.State.LOST);
-        }
-        handle.disconnect();


why we don't disconnect now?

See #20297 (review)

cloud-fan · 2018-01-19T02:32:33Z

launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java

+
+      synchronized (this) {
+        super.close();
+        notifyAll();


why notifyAll? who might be waiting?

gengliangwang · 2018-01-19T06:52:32Z

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) {


sameeragarwal · 2018-01-19T09:49:23Z

I've kicked a bunch of test builds in parallel to further rule out any flakiness on jenkins. Thanks!

SparkQA · 2018-01-19T12:48:43Z

Test build #4061 has finished for PR 20297 at commit 8bde21a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-19T13:52:27Z

Test build #4060 has finished for PR 20297 at commit 8bde21a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-19T14:07:44Z

Test build #4062 has finished for PR 20297 at commit 8bde21a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-01-19T18:21:44Z

I kicked an extra couple of builds aside from the one that should auto-trigger.

SparkQA · 2018-01-19T22:13:52Z

Test build #86395 has finished for PR 20297 at commit 95bac27.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-19T22:23:32Z

Test build #4066 has finished for PR 20297 at commit 95bac27.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-19T22:29:07Z

Test build #4067 has finished for PR 20297 at commit 95bac27.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-01-20T00:48:23Z

retest this please

SparkQA · 2018-01-20T04:45:45Z

Test build #86405 has finished for PR 20297 at commit 95bac27.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-20T04:51:02Z

Test build #4068 has finished for PR 20297 at commit 95bac27.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-01-22T05:27:40Z

LGTM

gengliangwang

LGTM

cloud-fan · 2018-01-22T06:49:23Z

thanks, merging to master/2.3!

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. The original version of this fix introduced the flipped version of the first race described above; the connection closing might override the handle state before the handle might have a chance to do cleanup. The fix there is to only dispose of the handle from the connection when there is an error, and let the handle dispose itself in the normal case. The fix also caused a bug in YarnClusterSuite to be surfaced; the code was checking for a file in the classpath that was not expected to be there in client mode. Because of the above issues, the error was not propagating correctly and the (buggy) test was incorrectly passing. Tested by running the existing unit tests a lot (and not seeing the errors I was seeing before). Author: Marcelo Vanzin <vanzin@cloudera.com> Closes #20297 from vanzin/SPARK-23020. (cherry picked from commit ec22897) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

## What changes were proposed in this pull request? This is a follow-up of #20297 which broke lint-java checks. This pr fixes the lint-java issues. ``` [ERROR] src/test/java/org/apache/spark/launcher/BaseSuite.java:[21,8] (imports) UnusedImports: Unused import - java.util.concurrent.TimeUnit. [ERROR] src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java:[27,8] (imports) UnusedImports: Unused import - java.util.concurrent.TimeUnit. ``` ## How was this patch tested? Checked manually in my local environment. Author: Takuya UESHIN <ueshin@databricks.com> Closes #20376 from ueshin/issues/SPARK-23020/fup1. (cherry picked from commit 8c273b4) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

## What changes were proposed in this pull request? This is a follow-up of apache#20297 which broke lint-java checks. This pr fixes the lint-java issues. ``` [ERROR] src/test/java/org/apache/spark/launcher/BaseSuite.java:[21,8] (imports) UnusedImports: Unused import - java.util.concurrent.TimeUnit. [ERROR] src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java:[27,8] (imports) UnusedImports: Unused import - java.util.concurrent.TimeUnit. ``` ## How was this patch tested? Checked manually in my local environment. Author: Takuya UESHIN <ueshin@databricks.com> Closes apache#20376 from ueshin/issues/SPARK-23020/fup1.

vanzin commented Jan 17, 2018

View reviewed changes

cloud-fan mentioned this pull request Jan 18, 2018

revert [SPARK-10030] Use tags to control which tests to run depending on changes being tested #20310

Closed

cloud-fan reviewed Jan 19, 2018

View reviewed changes

gengliangwang reviewed Jan 19, 2018

View reviewed changes

Feedback.

95bac27

gengliangwang approved these changes Jan 22, 2018

View reviewed changes

asfgit closed this in ec22897 Jan 22, 2018

vanzin deleted the SPARK-23020 branch January 22, 2018 21:37

ueshin mentioned this pull request Jan 24, 2018

[SPARK-23020][CORE][FOLLOWUP] Fix Java style check issues. #20376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-23020][CORE] Fix races in launcher code, test. #20297

[SPARK-23020][CORE] Fix races in launcher code, test. #20297

vanzin commented Jan 17, 2018 •

edited

Loading

vanzin Jan 17, 2018 •

edited

Loading

vanzin Jan 17, 2018

vanzin commented Jan 17, 2018

SparkQA commented Jan 17, 2018

vanzin commented Jan 18, 2018

SparkQA commented Jan 18, 2018

cloud-fan Jan 19, 2018

gengliangwang Jan 19, 2018

vanzin Jan 19, 2018

cloud-fan Jan 22, 2018

vanzin Jan 22, 2018

cloud-fan Jan 23, 2018

cloud-fan Jan 19, 2018

vanzin Jan 19, 2018

cloud-fan Jan 19, 2018

vanzin Jan 19, 2018

cloud-fan Jan 19, 2018

vanzin Jan 19, 2018

gengliangwang Jan 19, 2018

sameeragarwal commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

vanzin commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

vanzin commented Jan 20, 2018

SparkQA commented Jan 20, 2018

SparkQA commented Jan 20, 2018

cloud-fan commented Jan 22, 2018

gengliangwang left a comment

cloud-fan commented Jan 22, 2018

[SPARK-23020][CORE] Fix races in launcher code, test. #20297

[SPARK-23020][CORE] Fix races in launcher code, test. #20297

Conversation

vanzin commented Jan 17, 2018 • edited Loading

vanzin Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin commented Jan 17, 2018

SparkQA commented Jan 17, 2018

vanzin commented Jan 18, 2018

SparkQA commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameeragarwal commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

vanzin commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

vanzin commented Jan 20, 2018

SparkQA commented Jan 20, 2018

SparkQA commented Jan 20, 2018

cloud-fan commented Jan 22, 2018

gengliangwang left a comment

Choose a reason for hiding this comment

cloud-fan commented Jan 22, 2018

vanzin commented Jan 17, 2018 •

edited

Loading

vanzin Jan 17, 2018 •

edited

Loading