[SPARK-21991][LAUNCHER] Fix race condition in LauncherServer#acceptConnections #19217

nivox · 2017-09-13T09:16:30Z

What changes were proposed in this pull request?

This patch changes the order in which acceptConnections starts the client thread and schedules the client timeout action ensuring that the latter has been scheduled before the former get a chance to cancel it.

How was this patch tested?

Due to the non-deterministic nature of the patch I wasn't able to add a new test for this issue.

…21991)

vanzin · 2017-09-13T23:10:05Z

ok to test

SparkQA · 2017-09-14T02:18:35Z

Test build #81741 has finished for PR 19217 at commit 72af1aa.

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

srowen · 2017-10-24T08:42:49Z

Looks reasonable to me. @vanzin was this OK with you?

ash211

Thanks for getting this started @nivox ! I'm seeing it on a cluster too so am interested in getting this merged in to Apache.

ash211 · 2017-10-24T08:46:39Z

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

-            timeout.run();
-          }
+        synchronized (clients) {
+          clients.add(clientConnection);


we are now adding to clients before starting the clientThread instead of after. What's the expected ordering for these two operations?

Adding the connection to clients before starting the thread should be safer since otherwise the thread could have a chance of running and terminating before the connection is added to clients. This would cause the addition of an already terminated connection to the clients list which nobody would ever cleanup (i.e. memory leak).

This situation is really unlikely but still a possibility.

Changing the order of operation shouldn't affect any logic since the clients list is only used for cleanup in the close method.

ash211 · 2017-10-24T08:50:20Z

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

        };
        ServerConnection clientConnection = new ServerConnection(client, timeout);
        Thread clientThread = factory.newThread(clientConnection);
-        synchronized (timeout) {


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

This was exactly my reasoning. I couldn't find any reason why the synchronisation was needed. The removal was only a means to make the code simpler removing the cognitive cost suggested by the presence of a synchronise keyword. The actual fix doesn't depend on it.

ash211 · 2017-10-24T08:50:35Z

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

+        // 0 is used for testing to avoid issues with clock resolution / thread scheduling,
+        // and force an immediate timeout.
+        if (timeoutMs > 0) {
+          timeoutTimer.schedule(timeout, timeoutMs);


+1 on not calling getConnectionTimeout() multiple times

vanzin · 2017-10-24T18:46:26Z

The code LGTM, but the PR title should describe the fix, not the bug.

ash211 · 2017-10-24T22:01:41Z

How about [SPARK-21991][LAUNCHER] Fix race condition in LauncherServer#acceptConnections ?

vanzin · 2017-10-24T22:05:16Z

That sounds better.

ash211 · 2017-10-24T22:21:16Z

@nivox can you please update the PR title when you get the chance?

nivox · 2017-10-25T06:41:10Z

@vanzin @ash211 I just modified the title of the PR as per your suggestion

vanzin · 2017-10-25T17:10:00Z

Merging to master / 2.2 / 2.1.

…nnections ## What changes were proposed in this pull request? This patch changes the order in which _acceptConnections_ starts the client thread and schedules the client timeout action ensuring that the latter has been scheduled before the former get a chance to cancel it. ## How was this patch tested? Due to the non-deterministic nature of the patch I wasn't able to add a new test for this issue. Author: Andrea zito <[email protected]> Closes #19217 from nivox/SPARK-21991. (cherry picked from commit 6ea8a56) Signed-off-by: Marcelo Vanzin <[email protected]>

…nnections ## What changes were proposed in this pull request? This patch changes the order in which _acceptConnections_ starts the client thread and schedules the client timeout action ensuring that the latter has been scheduled before the former get a chance to cancel it. ## How was this patch tested? Due to the non-deterministic nature of the patch I wasn't able to add a new test for this issue. Author: Andrea zito <[email protected]> Closes #19217 from nivox/SPARK-21991. (cherry picked from commit 6ea8a56)

vanzin · 2017-10-25T17:14:53Z

Merged to 2.0 also.

I have a feeling that maven builds might fail because I noticed some trailing whitespace when looking at the raw patch...

ash211 · 2017-10-25T17:58:39Z

========================================================================
Running Java style checks
========================================================================
Using `mvn` from path: /home/ubuntu/spark/build/apache-maven-3.5.0/bin/mvn
Checkstyle checks failed at following occurrences:
[ERROR] src/main/java/org/apache/spark/launcher/LauncherServer.java:[238] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/main/java/org/apache/spark/launcher/LauncherServer.java:[247] (regexp) RegexpSingleline: No trailing whitespace allowed.
[error] running /home/ubuntu/spark/dev/lint-java ; received return code 1

will send a followup PR

ash211 · 2017-10-25T18:03:23Z

#19574

…nnections ## What changes were proposed in this pull request? This patch changes the order in which _acceptConnections_ starts the client thread and schedules the client timeout action ensuring that the latter has been scheduled before the former get a chance to cancel it. ## How was this patch tested? Due to the non-deterministic nature of the patch I wasn't able to add a new test for this issue. Author: Andrea zito <[email protected]> Closes apache#19217 from nivox/SPARK-21991. (cherry picked from commit 6ea8a56) Signed-off-by: Marcelo Vanzin <[email protected]>

Client timeout action scheduled before starting client thread (SPARK-…

72af1aa

…21991)

ash211 reviewed Oct 24, 2017

View reviewed changes

ash211 approved these changes Oct 24, 2017

View reviewed changes

nivox changed the title ~~[SPARK-21991][LAUNCHER] LauncherServer acceptConnections thread sometime dies if machine has very high load~~ [SPARK-21991][LAUNCHER] Fix race condition in LauncherServer#acceptConnections Oct 25, 2017

asfgit closed this in 6ea8a56 Oct 25, 2017

[SPARK-21991][LAUNCHER] Fix race condition in LauncherServer#acceptConnections #19217

[SPARK-21991][LAUNCHER] Fix race condition in LauncherServer#acceptConnections #19217

Uh oh!

Conversation

nivox commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

vanzin commented Sep 13, 2017

Uh oh!

SparkQA commented Sep 14, 2017

Uh oh!

srowen commented Oct 24, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

ash211 Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

nivox Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

nivox Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

vanzin commented Oct 24, 2017

Uh oh!

ash211 commented Oct 24, 2017

Uh oh!

vanzin commented Oct 24, 2017

Uh oh!

ash211 commented Oct 24, 2017

Uh oh!

nivox commented Oct 25, 2017

Uh oh!

vanzin commented Oct 25, 2017

Uh oh!

vanzin commented Oct 25, 2017

Uh oh!

ash211 commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ash211 commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nivox commented Sep 13, 2017 •

edited

Loading

ash211 commented Oct 25, 2017 •

edited

Loading