[SPARK-27338][Core] Fix deadlock in UnsafeExternalSorter.SpillableIterator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager #24265

venkata91 · 2019-04-01T17:47:55Z

What changes were proposed in this pull request?

In UnsafeExternalSorter.SpillableIterator#loadNext() takes lock on the UnsafeExternalSorter and calls freePage once the lastPage is consumed which needs to take a lock on TaskMemoryManager. At the same time, there can be another MemoryConsumer using UnsafeExternalSorter as part of sorting can try to allocatePage needs to get lock on TaskMemoryManager which can cause spill to happen which requires lock on UnsafeExternalSorter again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265.

To fix this, we can move the freePage call in loadNext outside of Synchronized block similar to the fix in SPARK-26265

How was this patch tested?

Manual tests were being done and will also try to add a test.

beliefer · 2019-04-02T02:40:07Z

I checked the code and found UnsafeExternalSorter.split will call the SpillableIterator.split.
SpillableIterator.split need to get lock on UnsafeExternalSorter.
UnsafeExternalSorter.loadNext will call the MemoryConsumer.freePage and MemoryConsumer.freePage will call the TaskMemoryManager.freePage.
TaskMemoryManager.freePage need to get lock on TaskMemoryManager.
I think your PR seems good!

beliefer · 2019-04-02T02:40:19Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

-          if (lastPage != null) {
-            freePage(lastPage);
-            lastPage = null;
+      MemoryBlock pageToFree = null;


I think your PR seems good!

cloud-fan · 2019-04-02T13:24:10Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

+          if (nextUpstream != null) {
+            // Just consumed the last record from in memory iterator
+            if(lastPage != null) {
+              pageToFree = lastPage;


can we add some comment to explain it? You can just copy it from https://github.com/apache/spark/pull/24269/files#diff-027299fb14327ddcaba457f81ecff32cR583

Sure @cloud-fan, will add and update the PR

Done @cloud-fan . I didn't know that you also filed a PR simultaneously for the same issue. :) Thanks for graciously closing your PR and accepting this PR.

attilapiros

+1 for adding some explanation (the comment mentioned by @cloud-fan) otherwise LGTM

cloud-fan · 2019-04-02T13:50:18Z

ok to test

jiangxb1987

LGTM, please update the comment.

SparkQA · 2019-04-02T18:47:53Z

Test build #104215 has finished for PR 24265 at commit 6a9de27.

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

SparkQA · 2019-04-03T21:17:09Z

Test build #104255 has finished for PR 24265 at commit 229cfae.

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

…rator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager ## What changes were proposed in this pull request? In `UnsafeExternalSorter.SpillableIterator#loadNext()` takes lock on the `UnsafeExternalSorter` and calls `freePage` once the `lastPage` is consumed which needs to take a lock on `TaskMemoryManager`. At the same time, there can be another MemoryConsumer using `UnsafeExternalSorter` as part of sorting can try to `allocatePage` needs to get lock on `TaskMemoryManager` which can cause spill to happen which requires lock on `UnsafeExternalSorter` again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265. To fix this, we can move the `freePage` call in `loadNext` outside of `Synchronized` block similar to the fix in SPARK-26265 ## How was this patch tested? Manual tests were being done and will also try to add a test. Closes #24265 from venkata91/deadlock-sorter. Authored-by: Venkata krishnan Sowrirajan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6c4552c) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2019-04-04T02:04:18Z

thanks, merging to master/2.4/2.3!

…rator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager ## What changes were proposed in this pull request? In `UnsafeExternalSorter.SpillableIterator#loadNext()` takes lock on the `UnsafeExternalSorter` and calls `freePage` once the `lastPage` is consumed which needs to take a lock on `TaskMemoryManager`. At the same time, there can be another MemoryConsumer using `UnsafeExternalSorter` as part of sorting can try to `allocatePage` needs to get lock on `TaskMemoryManager` which can cause spill to happen which requires lock on `UnsafeExternalSorter` again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265. To fix this, we can move the `freePage` call in `loadNext` outside of `Synchronized` block similar to the fix in SPARK-26265 ## How was this patch tested? Manual tests were being done and will also try to add a test. Closes #24265 from venkata91/deadlock-sorter. Authored-by: Venkata krishnan Sowrirajan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6c4552c) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? #24265 breaks the lint check, because it has trailing space. (not sure why it passed jenkins). This PR fixes it. ## How was this patch tested? N/A Closes #24289 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…rator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager ## What changes were proposed in this pull request? In `UnsafeExternalSorter.SpillableIterator#loadNext()` takes lock on the `UnsafeExternalSorter` and calls `freePage` once the `lastPage` is consumed which needs to take a lock on `TaskMemoryManager`. At the same time, there can be another MemoryConsumer using `UnsafeExternalSorter` as part of sorting can try to `allocatePage` needs to get lock on `TaskMemoryManager` which can cause spill to happen which requires lock on `UnsafeExternalSorter` again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265. To fix this, we can move the `freePage` call in `loadNext` outside of `Synchronized` block similar to the fix in SPARK-26265 ## How was this patch tested? Manual tests were being done and will also try to add a test. Closes apache#24265 from venkata91/deadlock-sorter. Authored-by: Venkata krishnan Sowrirajan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6c4552c) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? apache#24265 breaks the lint check, because it has trailing space. (not sure why it passed jenkins). This PR fixes it. ## How was this patch tested? N/A Closes apache#24289 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…rator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager ## What changes were proposed in this pull request? In `UnsafeExternalSorter.SpillableIterator#loadNext()` takes lock on the `UnsafeExternalSorter` and calls `freePage` once the `lastPage` is consumed which needs to take a lock on `TaskMemoryManager`. At the same time, there can be another MemoryConsumer using `UnsafeExternalSorter` as part of sorting can try to `allocatePage` needs to get lock on `TaskMemoryManager` which can cause spill to happen which requires lock on `UnsafeExternalSorter` again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265. To fix this, we can move the `freePage` call in `loadNext` outside of `Synchronized` block similar to the fix in SPARK-26265 ## How was this patch tested? Manual tests were being done and will also try to add a test. Closes apache#24265 from venkata91/deadlock-sorter. Authored-by: Venkata krishnan Sowrirajan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6c4552c) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? apache#24265 breaks the lint check, because it has trailing space. (not sure why it passed jenkins). This PR fixes it. ## How was this patch tested? N/A Closes apache#24289 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…rator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager ## What changes were proposed in this pull request? In `UnsafeExternalSorter.SpillableIterator#loadNext()` takes lock on the `UnsafeExternalSorter` and calls `freePage` once the `lastPage` is consumed which needs to take a lock on `TaskMemoryManager`. At the same time, there can be another MemoryConsumer using `UnsafeExternalSorter` as part of sorting can try to `allocatePage` needs to get lock on `TaskMemoryManager` which can cause spill to happen which requires lock on `UnsafeExternalSorter` again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265. To fix this, we can move the `freePage` call in `loadNext` outside of `Synchronized` block similar to the fix in SPARK-26265 ## How was this patch tested? Manual tests were being done and will also try to add a test. Closes apache#24265 from venkata91/deadlock-sorter. Authored-by: Venkata krishnan Sowrirajan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6c4552c) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? apache#24265 breaks the lint check, because it has trailing space. (not sure why it passed jenkins). This PR fixes it. ## How was this patch tested? N/A Closes apache#24289 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? apache/spark#24265 breaks the lint check, because it has trailing space. (not sure why it passed jenkins). This PR fixes it. ## How was this patch tested? N/A Closes #24289 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit af0a4bb)

## What changes were proposed in this pull request? apache/spark#24265 breaks the lint check, because it has trailing space. (not sure why it passed jenkins). This PR fixes it. ## How was this patch tested? N/A Closes #24289 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…rator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager In `UnsafeExternalSorter.SpillableIterator#loadNext()` takes lock on the `UnsafeExternalSorter` and calls `freePage` once the `lastPage` is consumed which needs to take a lock on `TaskMemoryManager`. At the same time, there can be another MemoryConsumer using `UnsafeExternalSorter` as part of sorting can try to `allocatePage` needs to get lock on `TaskMemoryManager` which can cause spill to happen which requires lock on `UnsafeExternalSorter` again causing deadlock. This is a classic deadlock situation happening similar to the SPARK-26265. To fix this, we can move the `freePage` call in `loadNext` outside of `Synchronized` block similar to the fix in SPARK-26265 Manual tests were being done and will also try to add a test. Closes apache#24265 from venkata91/deadlock-sorter. Authored-by: Venkata krishnan Sowrirajan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6c4552c) Signed-off-by: Wenchen Fan <[email protected]> Ref: LIHADOOP-58062 RB=2553586 BUG=LIHADOOP-58062 G=spark-reviewers R=mmuralid A=mmuralid

Fix deadlock while freeing page

6a9de27

beliefer reviewed Apr 2, 2019

View reviewed changes

cloud-fan reviewed Apr 2, 2019

View reviewed changes

attilapiros reviewed Apr 2, 2019

View reviewed changes

jiangxb1987 approved these changes Apr 2, 2019

View reviewed changes

Addressed review comments

229cfae

cloud-fan closed this in 6c4552c Apr 4, 2019

cloud-fan mentioned this pull request Apr 4, 2019

[SPARK-27338][CORE][FOLLOWUP] remove trailing space #24289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-27338][Core] Fix deadlock in UnsafeExternalSorter.SpillableIterator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager #24265

[SPARK-27338][Core] Fix deadlock in UnsafeExternalSorter.SpillableIterator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager #24265

Uh oh!

venkata91 commented Apr 1, 2019 •

edited

Loading

Uh oh!

beliefer commented Apr 2, 2019

Uh oh!

beliefer Apr 2, 2019

Uh oh!

cloud-fan Apr 2, 2019

Uh oh!

venkata91 Apr 2, 2019

Uh oh!

venkata91 Apr 3, 2019 •

edited

Loading

Uh oh!

attilapiros left a comment

Uh oh!

cloud-fan commented Apr 2, 2019

Uh oh!

jiangxb1987 left a comment

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 3, 2019

Uh oh!

cloud-fan commented Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-27338][Core] Fix deadlock in UnsafeExternalSorter.SpillableIterator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager #24265

[SPARK-27338][Core] Fix deadlock in UnsafeExternalSorter.SpillableIterator when locking both UnsafeExternalSorter.SpillableIterator and TaskMemoryManager #24265

Uh oh!

Conversation

venkata91 commented Apr 1, 2019 • 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!

beliefer commented Apr 2, 2019

Uh oh!

beliefer Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

venkata91 Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

venkata91 Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attilapiros left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 2, 2019

Uh oh!

jiangxb1987 left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 3, 2019

Uh oh!

cloud-fan commented Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

venkata91 commented Apr 1, 2019 •

edited

Loading

venkata91 Apr 3, 2019 •

edited

Loading