[SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory #3574

suyanNone · 2014-12-03T07:35:56Z

[SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory

After got info lock in one of them: removeBlock/dropOldBlocks/dropFromMemory method in BlockManager, the block that info represented may have already removed.

The Three method have the same logic to got info lock:

   info = blockInfo.get(id)
   if (info != null) {
     info.synchronized {
       do sth
     } 
   }

So it is chanced that when one thread got info.synchronized but it already removed from other threads who got info.synchronized before him.

In removeBlock and dropOldBlocks is idempotent if they not have blockinfo.get(blockId) != null
But in dropFromMemory it may be problematic for it may drop block data(which already removed) into diskstore, this method calls data store operations that should not handle missing blocks.

AmplabJenkins · 2014-12-03T07:37:11Z

Can one of the admins verify this patch?

JoshRosen · 2014-12-05T02:50:46Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

-        if (!removedFromMemory && !removedFromDisk && !removedFromTachyon) {
-          logWarning(s"Block $blockId could not be removed as it was not found in either " +
-            "the disk, memory, or tachyon store")
+        if (blockInfo.get(blockId).isEmpty) {


Unless I'm missing something, this seems like the opposite of the condition we want here: if blockInfo doesn't contain an entry for blockId, then why would we want to remove that entry?

ThreadA into RemoveBlock(), and got info for blockId1

ThreadB into DropFromMemory(), and got info for blockId1

now Thread A, B all want got info.sychronized

B got, and drop block(blockId) from memory, and this block is use memory only, so it remove from blockinfo. and then release the lock.

then A got, but info is already not in blockinfo.

Did I have miss sth or misunderstand sth? may dropForMemory and removeBlock can't happen at the same time?

I don't think that this extra check is necessary; check out my comment on the main pull request and see if you agree.

Even if we did want to add a check here, I think we want to check for if(blockInfo.get(blockId).nonEmpty), since this branch handles the case where blocks have not been removed already.

JoshRosen · 2014-12-05T03:54:32Z

Can you elaborate on the problem that this PR addresses? Have you observed an actual error that this fixes?

suyanNone · 2014-12-05T11:40:13Z

@JoshRosen
ThreadA into RemoveBlock(), and got info for blockId1
ThreadB into DropFromMemory(), and got info for blockId1
now Thread A, B all want got info.sychronized

B got, and drop block(blockId) from memory, and this block is use memory only, so it remove from blockinfo. and then release the lock.

then A got, but info is already not in blockinfo.

Did I have miss sth or misunderstand sth? may dropForMemory and removeBlock can't happen at the same time?

JoshRosen · 2014-12-06T19:43:09Z

Ah, I think I see your concern: let's say that we a block and there are two threads that are racing to perform operations on it: to use your example, thread A wants to call removeBlock() and thread B wants to call dropFromMemory(). For this code to work correctly, we want it to operate correctly for all possible interleavings of those threads

If we consider the case where all of thread A's steps execute before any of thread B's, then things work okay: thread A will have removed the entry from blockInfo before thread B runs, so B will see that info == null and log a warning that the block has already been removed. The same is true for B before A.

In another execution, though, both threads could find the BlockInfo instance in the blockInfo map but race on acquiring its lock (info.synchronized), so info != null will be true for both threads. I agree that this could be a problem, but it might not be if the operations performed in those threads are idempotent. Let's take a look and see if that's the case:

removeBlock: all of the operations here are idempotent, so at worst we get a warning if we run this on a block that's removed by another racing thread.
dropOldBlocks: similarly, this just consists of calls to *Store.remove(), which are idempotent.
dropFromMemory: this case might actually be problematic, since I think that this method calls data store operations that don't handle missing blocks. I'm going to look at this case in a little more detail, but I think that your fix for this might be a good idea.

JoshRosen · 2014-12-06T19:47:12Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

-          if (level.useOffHeap) { tachyonStore.remove(id) }
-          iterator.remove()
-          logInfo(s"Dropped block $id")
+          if (blockInfo.get(id).isEmpty) {


Similarly, I don't think that we strictly need a check here since the remove(id) operations are idempotent. It might be nice to log a warning if the block has already been removed, but that might not be necessary since this is a background cleanup call.

suyanNone · 2014-12-08T05:14:04Z

@JoshRosen
yes, it will not cause any error if there not have a check in removeBlock and dropOldBlocks
I pull this request, because I think it will be more reasonable in semantic.
and I already remove the check in removeBlock and dropOldBlocks

JoshRosen · 2014-12-08T18:35:35Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

+        if (blockInfo.get(blockId).isEmpty) {
+          logWarning(s"Block $blockId was already dropped.")
+          return None
+        } else if(!info.waitForReady()) {


Minor style nit: this needs a space after the if and before the open paren: if (!info....

JoshRosen · 2014-12-08T18:35:42Z

Jenkins, this is ok to test.

SparkQA · 2014-12-08T18:40:14Z

Test build #24225 has started for PR 3574 at commit 55fa4ba.

This patch merges cleanly.

SparkQA · 2014-12-08T20:05:42Z

Test build #24225 has finished for PR 3574 at commit 55fa4ba.

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

AmplabJenkins · 2014-12-08T20:05:45Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24225/
Test PASSed.

JoshRosen · 2014-12-08T22:21:49Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

@@ -1010,7 +1010,10 @@ private[spark] class BlockManager(
      info.synchronized {
        // required ? As of now, this will be invoked only for blocks which are ready


This comment actually refers to the !info.waitForReady() case, so I'd like to either move the comment or swap the order of these checks so that we check for blockInfo.get(blockId).isEmpty in the else if clause instead.

JoshRosen · 2014-12-08T22:23:13Z

Left one minor code organization comment; aside from that, this looks good to me and should be ready to merge after you fix that up (I can do it if you don't have time, though; just let me know).

There are a couple of edits that I'd like to make to the commit title / description before merging this, but I can do it myself on merge.

Thanks for the careful analysis and for catching this issue!

suyanNone · 2014-12-09T02:24:17Z

@JoshRosen I refine the code according your comments.
If still have problem, it OK for you to fix up including the title and comments, and thanks for you to check my code and give suggestion.

SparkQA · 2014-12-09T02:30:29Z

Test build #24234 has started for PR 3574 at commit edb989d.

This patch merges cleanly.

SparkQA · 2014-12-09T03:53:01Z

Test build #24234 has finished for PR 3574 at commit edb989d.

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

AmplabJenkins · 2014-12-09T03:53:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24234/
Test PASSed.

JoshRosen · 2014-12-09T23:10:52Z

LGTM. I like how this ended up evolving into a very small, focused fix.

I'm going to fix up the title / commit message and merge this into master. Even though I don't think we've observed failures from this in the wild, I'm in favor of backporting this into the maintenance branches, too, since it probably won't cause any harm to add more defensive checks here. I'll merge this into branch-1.0 and branch-1.1 and hold off on branch-1.2 for now (I'll add a backport-needed JIRA label and pull this in after 1.2.0 is released).

Thanks!

… has been removed after synchronizing on BlockInfo instance. After synchronizing on the `info` lock in the `removeBlock`/`dropOldBlocks`/`dropFromMemory` methods in BlockManager, the block that `info` represented may have already removed. The three methods have the same logic to get the `info` lock: ``` info = blockInfo.get(id) if (info != null) { info.synchronized { // do something } } ``` So, there is chance that when a thread enters the `info.synchronized` block, `info` has already been removed from the `blockInfo` map by some other thread who entered `info.synchronized` first. The `removeBlock` and `dropOldBlocks` methods are idempotent, so it's safe for them to run on blocks that have already been removed. But in `dropFromMemory` it may be problematic since it may drop block data which already removed into the diskstore, and this calls data store operations that are not designed to handle missing blocks. This patch fixes this issue by adding a check to `dropFromMemory` to test whether blocks have been removed by a racing thread. Author: hushan[胡珊] <hushan@xiaomi.com> Closes #3574 from suyanNone/refine-block-concurrency and squashes the following commits: edb989d [hushan[胡珊]] Refine code style and comments position 55fa4ba [hushan[胡珊]] refine code e57e270 [hushan[胡珊]] add check info is already remove or not while having gotten info.syn (cherry picked from commit 30dca92) Signed-off-by: Josh Rosen <joshrosen@databricks.com>

JoshRosen · 2014-12-17T20:17:09Z

I've merged this into branch-1.2.

… has been removed after synchronizing on BlockInfo instance. After synchronizing on the `info` lock in the `removeBlock`/`dropOldBlocks`/`dropFromMemory` methods in BlockManager, the block that `info` represented may have already removed. The three methods have the same logic to get the `info` lock: ``` info = blockInfo.get(id) if (info != null) { info.synchronized { // do something } } ``` So, there is chance that when a thread enters the `info.synchronized` block, `info` has already been removed from the `blockInfo` map by some other thread who entered `info.synchronized` first. The `removeBlock` and `dropOldBlocks` methods are idempotent, so it's safe for them to run on blocks that have already been removed. But in `dropFromMemory` it may be problematic since it may drop block data which already removed into the diskstore, and this calls data store operations that are not designed to handle missing blocks. This patch fixes this issue by adding a check to `dropFromMemory` to test whether blocks have been removed by a racing thread. Author: hushan[胡珊] <hushan@xiaomi.com> Closes #3574 from suyanNone/refine-block-concurrency and squashes the following commits: edb989d [hushan[胡珊]] Refine code style and comments position 55fa4ba [hushan[胡珊]] refine code e57e270 [hushan[胡珊]] add check info is already remove or not while having gotten info.syn (cherry picked from commit 30dca92) Signed-off-by: Josh Rosen <joshrosen@databricks.com>

add check info is already remove or not while having gotten info.syn

e57e270

JoshRosen reviewed Dec 5, 2014
View reviewed changes

JoshRosen reviewed Dec 6, 2014
View reviewed changes

refine code

55fa4ba

suyanNone force-pushed the refine-block-concurrency branch from 0c1a249 to 55fa4ba Compare December 8, 2014 05:10

JoshRosen reviewed Dec 8, 2014
View reviewed changes

suyanNone changed the title ~~[SPARK-4714][CORE]: Add checking info is null or not while having gotten info.syn~~ [SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory Dec 9, 2014

Refine code style and comments position

edb989d

asfgit closed this in 30dca92 Dec 9, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory #3574

[SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory #3574

suyanNone commented Dec 3, 2014

AmplabJenkins commented Dec 3, 2014

JoshRosen Dec 5, 2014

suyanNone Dec 5, 2014

JoshRosen Dec 6, 2014

JoshRosen commented Dec 5, 2014

suyanNone commented Dec 5, 2014

JoshRosen commented Dec 6, 2014

JoshRosen Dec 6, 2014

suyanNone commented Dec 8, 2014

JoshRosen Dec 8, 2014

JoshRosen commented Dec 8, 2014

SparkQA commented Dec 8, 2014

SparkQA commented Dec 8, 2014

AmplabJenkins commented Dec 8, 2014

JoshRosen Dec 8, 2014

JoshRosen commented Dec 8, 2014

suyanNone commented Dec 9, 2014

SparkQA commented Dec 9, 2014

SparkQA commented Dec 9, 2014

AmplabJenkins commented Dec 9, 2014

JoshRosen commented Dec 9, 2014

JoshRosen commented Dec 17, 2014

		@@ -1010,7 +1010,10 @@ private[spark] class BlockManager(
		info.synchronized {
		// required ? As of now, this will be invoked only for blocks which are ready

[SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory #3574

[SPARK-4714][CORE]: Check block have removed or not after got info lock in dropFromMemory #3574

Conversation

suyanNone commented Dec 3, 2014

AmplabJenkins commented Dec 3, 2014

JoshRosen Dec 5, 2014

Choose a reason for hiding this comment

suyanNone Dec 5, 2014

Choose a reason for hiding this comment

JoshRosen Dec 6, 2014

Choose a reason for hiding this comment

JoshRosen commented Dec 5, 2014

suyanNone commented Dec 5, 2014

JoshRosen commented Dec 6, 2014

JoshRosen Dec 6, 2014

Choose a reason for hiding this comment

suyanNone commented Dec 8, 2014

JoshRosen Dec 8, 2014

Choose a reason for hiding this comment

JoshRosen commented Dec 8, 2014

SparkQA commented Dec 8, 2014

SparkQA commented Dec 8, 2014

AmplabJenkins commented Dec 8, 2014

JoshRosen Dec 8, 2014

Choose a reason for hiding this comment

JoshRosen commented Dec 8, 2014

suyanNone commented Dec 9, 2014

SparkQA commented Dec 9, 2014

SparkQA commented Dec 9, 2014

AmplabJenkins commented Dec 9, 2014

JoshRosen commented Dec 9, 2014

JoshRosen commented Dec 17, 2014