[SPARK-26265][Core] Fix deadlock in BytesToBytesMap.MapIterator when locking both BytesToBytesMap.MapIterator and TaskMemoryManager #23272

viirya · 2018-12-10T08:16:05Z

What changes were proposed in this pull request?

In BytesToBytesMap.MapIterator.advanceToNextPage, We will first lock this MapIterator and then TaskMemoryManager when going to free a memory page by calling freePage. At the same time, it is possibly that another memory consumer first locks TaskMemoryManager and then this MapIterator when it acquires memory and causes spilling on this MapIterator.

So it ends with the MapIterator object holds lock to the MapIterator object and waits for lock on TaskMemoryManager, and the other consumer holds lock to TaskMemoryManager and waits for lock on the MapIterator object.

To avoid deadlock here, this patch proposes to keep reference to the page to free and free it after releasing the lock of MapIterator.

How was this patch tested?

Added test and manually test by running the test 100 times to make sure there is no deadlock.

viirya · 2018-12-10T08:33:36Z

cc @cloud-fan

dongjoon-hyun · 2018-12-10T09:55:01Z

core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java

  }

+  @Test
+  public void avoidDeadlock() throws InterruptedException {


Hi, @viirya . Since this test case reproduces Deadlock situation, we need a timeout logic. Otherwise, it will hang (instead of failures) when we hit this issue later.

I've tried several ways to set a timeout logic, but don't work. The deadlock always hangs the test and timeout logic.

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

core/src/test/java/org/apache/spark/memory/TestMemoryConsumer.java

cloud-fan · 2018-12-10T11:45:58Z

have you seen any bug report caused by this dead lock?

SparkQA · 2018-12-10T12:09:05Z

Test build #99900 has finished for PR 23272 at commit 25e8e06.

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

viirya · 2018-12-10T12:13:16Z

have you seen any bug report caused by this dead lock?

The original reporter of the JIRA ticket SPARK-26265 has hit with this bug in their workload.

cloud-fan · 2018-12-10T12:20:46Z

core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java

+      assertFalse(iter.hasNext());
+    } finally {
+      map.free();
+      thread.join();


Is this line where the test hangs without the fix?

When without this line, the test still hangs. The test thread hangs on the deadlock with the other thread of running memoryConsumer.

This line just makes sure memoryConsumer to end and free acquired memory.

SparkQA · 2018-12-10T17:44:08Z

Test build #99914 has finished for PR 23272 at commit 4c621d2.

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

SparkQA · 2018-12-10T17:54:29Z

Test build #99915 has finished for PR 23272 at commit 9d52320.

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

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

viirya · 2018-12-11T03:28:07Z

I think the page is used exclusively by the map and the iterator. So it could not be freed by other consumer.

…

On Tue, Dec 11, 2018, 10:23 Wenchen Fan ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java <#23272 (comment)>: > @@ -283,6 +290,9 @@ private void advanceToNextPage() { } } } + if (pageToFree != null) { + freePage(pageToFree); is it possible that this page is already freed by another consumer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23272 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEM9-_lp7w_wK9mWUYjtPT1h9o5O0rzks5u3xcSgaJpZM4ZK2_Y> .

core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java

dongjoon-hyun · 2018-12-11T03:52:01Z

core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java

+
+    try {
+      int i;
+      for (i = 0; i < 1024; i++) {


~~Let's use for (int i = 0; ... here and line 708 because int i is not referenced outside of for loop.~~
Never mind. I found that this is the convention in this test suite.

viirya · 2018-12-11T04:21:51Z

Oh, you meant that the page is freed by other using this map or iterator. Is it a problem? I think it should not be a case that more than one consumers free the same page at the same time.

…

On Tue, Dec 11, 2018, 11:34 Wenchen Fan ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java <#23272 (comment)>: > @@ -283,6 +290,9 @@ private void advanceToNextPage() { } } } + if (pageToFree != null) { + freePage(pageToFree); the MapIterator.spill will be called by BytesToBytesMap.spill which will be called by other consumers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23272 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEM91AsLKsA5zS0gXEhip3GuUcVjJ7-ks5u3yfAgaJpZM4ZK2_Y> .

viirya · 2018-12-11T04:25:34Z

If you worry that the page is to be freed by the other consumer using the map iterator and also the map iterator itself, because I am not in front of laptop so I can't check it. But I guess freePage should already cover it.

…

On Tue, Dec 11, 2018, 11:34 Wenchen Fan ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java <#23272 (comment)>: > @@ -283,6 +290,9 @@ private void advanceToNextPage() { } } } + if (pageToFree != null) { + freePage(pageToFree); the MapIterator.spill will be called by BytesToBytesMap.spill which will be called by other consumers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23272 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEM91AsLKsA5zS0gXEhip3GuUcVjJ7-ks5u3yfAgaJpZM4ZK2_Y> .

cloud-fan · 2018-12-11T10:39:41Z

LGTM. last question: does the test always reproduce the bug? Or it has some randomness?

SparkQA · 2018-12-11T12:24:25Z

Test build #99956 has finished for PR 23272 at commit 0405527.

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

viirya · 2018-12-11T12:48:03Z

last question: does the test always reproduce the bug? Or it has some randomness?

If without the change, as I tried it locally 10 times, the test can reproduce the bug 10 times. But I'm not sure if it is 100% to reproduce the bug. I think we can't always to reproduce a deadlock like this.

cloud-fan · 2018-12-11T13:06:48Z

thanks, merging to master!

Can you send a new PR for 2.4 without the synchronized move around?

viirya · 2018-12-11T13:11:24Z

Can you send a new PR for 2.4 without the synchronized move around?

Ok. Thanks.

SparkQA · 2018-12-11T13:53:02Z

Test build #99966 has finished for PR 23272 at commit 0849083.

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

viirya · 2018-12-11T14:09:46Z

I think the failed tests are unrelated. cc @cloud-fan

## What changes were proposed in this pull request? Based on the [comment](#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes #23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 1b604c1) Signed-off-by: Hyukjin Kwon <[email protected]>

## What changes were proposed in this pull request? Based on the [comment](#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes #23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

## What changes were proposed in this pull request? Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

…locking both BytesToBytesMap.MapIterator and TaskMemoryManager ## What changes were proposed in this pull request? In `BytesToBytesMap.MapIterator.advanceToNextPage`, We will first lock this `MapIterator` and then `TaskMemoryManager` when going to free a memory page by calling `freePage`. At the same time, it is possibly that another memory consumer first locks `TaskMemoryManager` and then this `MapIterator` when it acquires memory and causes spilling on this `MapIterator`. So it ends with the `MapIterator` object holds lock to the `MapIterator` object and waits for lock on `TaskMemoryManager`, and the other consumer holds lock to `TaskMemoryManager` and waits for lock on the `MapIterator` object. To avoid deadlock here, this patch proposes to keep reference to the page to free and free it after releasing the lock of `MapIterator`. ## How was this patch tested? Added test and manually test by running the test 100 times to make sure there is no deadlock. Closes apache#23272 from viirya/SPARK-26265. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

## What changes were proposed in this pull request? Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 1b604c1) Signed-off-by: Hyukjin Kwon <[email protected]>

## What changes were proposed in this pull request? Based on the [comment](apache/spark#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes #23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 1b604c1) Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 6019d9a)

…locking both BytesToBytesMap.MapIterator and TaskMemoryManager In `BytesToBytesMap.MapIterator.advanceToNextPage`, We will first lock this `MapIterator` and then `TaskMemoryManager` when going to free a memory page by calling `freePage`. At the same time, it is possibly that another memory consumer first locks `TaskMemoryManager` and then this `MapIterator` when it acquires memory and causes spilling on this `MapIterator`. So it ends with the `MapIterator` object holds lock to the `MapIterator` object and waits for lock on `TaskMemoryManager`, and the other consumer holds lock to `TaskMemoryManager` and waits for lock on the `MapIterator` object. To avoid deadlock here, this patch proposes to keep reference to the page to free and free it after releasing the lock of `MapIterator`. Added test and manually test by running the test 100 times to make sure there is no deadlock. Closes apache#23272 from viirya/SPARK-26265. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry-picked from commit a3bbca9) [SPARK-26265][CORE][FOLLOWUP] Put freePage into a finally block Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry-picked from commit 1b604c1) Ref: LIHADOOP-43221 RB=1518143 BUG=LIHADOOP-43221 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=fli

Fix deadlock in BytesToBytesMap.

25e8e06

dongjoon-hyun reviewed Dec 10, 2018

View reviewed changes

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java Show resolved Hide resolved

cloud-fan reviewed Dec 10, 2018

View reviewed changes

core/src/test/java/org/apache/spark/memory/TestMemoryConsumer.java Outdated Show resolved Hide resolved

Covers OffHeap.

4c621d2

Remove VisibleForTesting.

9d52320

cloud-fan reviewed Dec 10, 2018

View reviewed changes

cloud-fan reviewed Dec 11, 2018

View reviewed changes

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java Show resolved Hide resolved

dongjoon-hyun reviewed Dec 11, 2018

View reviewed changes

core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Dec 11, 2018

View reviewed changes

viirya added 2 commits December 11, 2018 16:02

Shorten runnable.

0405527

Move synchronized.

0849083

asfgit closed this in a3bbca9 Dec 11, 2018

viirya mentioned this pull request Dec 12, 2018

[SPARK-26265][Core][Followup] Put freePage into a finally block #23294

Closed

viirya deleted the SPARK-26265 branch December 27, 2023 18:22

[SPARK-26265][Core] Fix deadlock in BytesToBytesMap.MapIterator when locking both BytesToBytesMap.MapIterator and TaskMemoryManager #23272

[SPARK-26265][Core] Fix deadlock in BytesToBytesMap.MapIterator when locking both BytesToBytesMap.MapIterator and TaskMemoryManager #23272

Uh oh!

Conversation

viirya commented Dec 10, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya commented Dec 10, 2018

Uh oh!

dongjoon-hyun Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Dec 10, 2018 • 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!

Uh oh!

Uh oh!

cloud-fan commented Dec 10, 2018

Uh oh!

SparkQA commented Dec 10, 2018

Uh oh!

viirya commented Dec 10, 2018

Uh oh!

cloud-fan Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Dec 10, 2018 • 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!

SparkQA commented Dec 10, 2018

Uh oh!

SparkQA commented Dec 10, 2018

Uh oh!

Uh oh!

viirya commented Dec 11, 2018 via email

Uh oh!

Uh oh!

dongjoon-hyun Dec 11, 2018 • 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!

viirya commented Dec 11, 2018 via email

Uh oh!

viirya commented Dec 11, 2018 via email

Uh oh!

cloud-fan commented Dec 11, 2018

Uh oh!

SparkQA commented Dec 11, 2018

Uh oh!

viirya commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Dec 11, 2018

Uh oh!

viirya commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 11, 2018

Uh oh!

viirya commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya Dec 10, 2018 •

edited

Loading

viirya Dec 10, 2018 •

edited

Loading

dongjoon-hyun Dec 11, 2018 •

edited

Loading

viirya commented Dec 11, 2018 •

edited

Loading

viirya commented Dec 11, 2018 •

edited

Loading