[SPARK-17816] [Core] Fix ConcurrentModificationException issue in BlockStatusesAccumulator by seyfe · Pull Request #15371 · apache/spark

seyfe · 2016-10-06T01:40:04Z

What changes were proposed in this pull request?

Change the BlockStatusesAccumulator to return immutable object when value method is called.

How was this patch tested?

Existing tests plus I verified this change by running a pipeline which consistently repro this issue.

This is the stack trace for this exception:
java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901) at java.util.ArrayList$Itr.next(ArrayList.java:851) at scala.collection.convert.Wrappers$JIteratorWrapper.next(Wrappers.scala:43) at scala.collection.Iterator$class.foreach(Iterator.scala:893) at scala.collection.AbstractIterator.foreach(Iterator.scala:1336) at scala.collection.IterableLike$class.foreach(IterableLike.scala:72) at scala.collection.AbstractIterable.foreach(Iterable.scala:54) at scala.collection.generic.Growable$class.$plus$plus$eq(Growable.scala:59) at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:183) at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:45) at scala.collection.TraversableLike$class.to(TraversableLike.scala:590) at scala.collection.AbstractTraversable.to(Traversable.scala:104) at scala.collection.TraversableOnce$class.toList(TraversableOnce.scala:294) at scala.collection.AbstractTraversable.toList(Traversable.scala:104) at org.apache.spark.util.JsonProtocol$.accumValueToJson(JsonProtocol.scala:314) at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291) at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291) at scala.Option.map(Option.scala:146) at org.apache.spark.util.JsonProtocol$.accumulableInfoToJson(JsonProtocol.scala:291) at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283) at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234) at scala.collection.immutable.List.foreach(List.scala:381) at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:35) at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45) at scala.collection.TraversableLike$class.map(TraversableLike.scala:234) at scala.collection.AbstractTraversable.map(Traversable.scala:104) at org.apache.spark.util.JsonProtocol$.taskInfoToJson(JsonProtocol.scala:283) at org.apache.spark.util.JsonProtocol$.taskEndToJson(JsonProtocol.scala:145) at org.apache.spark.util.JsonProtocol$.sparkEventToJson(JsonProtocol.scala:76)

hvanhovell · 2016-10-06T03:02:41Z

ok to test

SparkQA · 2016-10-06T05:21:48Z

Test build #66425 has finished for PR 15371 at commit 1934dc9.

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

seyfe · 2016-10-06T07:22:16Z

Unfortunately, this PR doesn't fix the java.util.ConcurrentModificationException. I can still repro it. I will spend more time on it tomorrow morning.

srowen

Yeah I'm not sure this is the issue. It's being modified while it's being serialized.

srowen · 2016-10-06T07:49:39Z

I don't see why this would help. You add a wrapper, but, synchronizing your local access to it doesn't do anything because nothing else is synchronizing on it.

PS can the List[(BlockId,BlockStatus)] type just be part of the match predicate?

srowen · 2016-10-06T07:51:51Z

What does this do -- just puts a copy before the work of mapping? I could see how that would tend to help.

This wasn't the root cause but it's something nice to have. If you prefer, I can revert this line.

Seems OK if it's related cleanup, and potentially helps a closely related manifestation

rxin · 2016-10-06T17:48:02Z

One comment: once you figured out the proper fix, please add some comments inline so they don't get accidentally removed in the future.

zsxwing · 2016-10-06T20:40:57Z

@seyfe Thanks for reporting this one. Actually, it's different from SPARK-17463. Could you create a new ticket for this issue, please? The cause is we send a mutable TaskInfo to listeners but we may still update TaskInfo's fields (e.g., accumulables) in another thread... Ideally, all events sent to the listeners should be immutable.

seyfe · 2016-10-06T22:21:03Z

Hi @zsxwing. I have a fix ready and testing it now. I will create a new ticket and send an updated PR today.

SparkQA · 2016-10-07T00:02:50Z

Test build #66473 has finished for PR 15371 at commit 5380aff.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-10-07T00:05:54Z

@seyfe The issue is TaskInfo.accumulables is accessed in multiple threads without synchronization. TaskMetrics.updatedBlockStatuses is fine.

seyfe · 2016-10-07T00:15:35Z

@zsxwing. BlockStatusesAccumulator is using synchronizedList which is _seq for concurrency. But when the value method is executed and the output is used by asScala.toList it caused the ConcurrentModificationException.

Collections.synchronizedList suggests using synchronized keyword when using its iterator which is done by asScala method. Does that make sense?

zsxwing · 2016-10-07T00:22:53Z

@zsxwing. BlockStatusesAccumulator is using synchronizedList which is _seq for concurrency. But when the value method is executed and the output is used by asScala.toList it caused the ConcurrentModificationException.

Collections.synchronizedList suggests using synchronized keyword when using its iterator which is done by asScala method. Does that make sense?

@seyfe The comment you are deleting explains why it's safe: the driver doesn't modify BlockStatusesAccumulator.

seyfe · 2016-10-07T00:32:38Z

@zsxwing . JArray(v.asInstanceOf[java.util.List[(BlockId, BlockStatus)]].asScala.toList.map this is the line of code which is causing ConcurrentModificationException. And you can see that from call stack. This is coming from BlockStatusesAccumulator. Am I missing something here?

seyfe · 2016-10-07T00:40:01Z

I also want to point out that below is the core part of fix. Rest of the code changes are side-effects of it.

+  // `asScala` accesses the internal values using `java.util.Iterator` so needs to be synchronized
 +  override def value: List[(BlockId, BlockStatus)] = {
 +    _seq.synchronized {
 +      _seq.asScala.toList
 +    }
 +  }

srowen

Dumb question but how big is this? the solution here is to copy the data structure, which is a good defensive move, as long as it's not big and nothing is actually relying on observing changes to the underlying data. Is that valid?

srowen · 2016-10-07T06:47:31Z

Seems OK if it's related cleanup, and potentially helps a closely related manifestation

seyfe · 2016-10-07T08:19:27Z

Hi @srowen,

This PR doesn't introduce any extra data copy operations. It moves the data copy code from JsonProtocol:accumValueToJson method to BlockStatusesAccumulator:value method so it can be called inside the synchronized block. But let me try to answer your question.

I checked the data size using 3 different pipelines. 99% of the time ArrayList has less than 4 items. There was one of case where it maxed at 4000 items but that was less than 1% of the time. I ran my test with 4000 executors, I think that is why this 4000 number came up.

I debated other options as well. Moving Json serialization into BlockStatusesAccumulator would help but I didn't want to mix match these classes. I considered introducing Akka Actors for thread safety but I think that would be an overkill in this scenario since data size is small. That is why I proposed this solution but I would love to hear your proposals.

I don't know the answer for the second part of your question (below), but existing behavior is not changed. Only change is that we can convert ArrayList to a Scala List inside a synchronized block so we won't get ConcurrentModificationException . If we do need to rely on observing changes, I think we do need to have a bigger change. @zsxwing, would you please comment on that part?

nothing is actually relying on observing changes to the underlying data

srowen · 2016-10-07T09:41:20Z

I see the new copy (of course or else this wouldn't help) but where is a copy removed? I'm probably overlooking it. A toList call was moved, which we discussed above briefly. I still think it's all fine, just worth double-checking both of our logics.

seyfe · 2016-10-07T17:01:27Z

Hi @srowen , this is the part that I removed extra copy operation. I changed this line because this conversion is already done by BlockStatusesAccumulator.

JArray(v.asInstanceOf[java.util.List[(BlockId, BlockStatus)]].asScala.toList.map {
with
JArray(v.map {

zsxwing · 2016-10-07T20:39:34Z

@seyfe I'm taking my words back. Yea, BlockStatusesAccumulator.merge will be called in driver.

zsxwing · 2016-10-07T20:49:29Z

_seq.synchronized is wrong. Collections.synchronizedList uses its internal mutex to lock instead of this.

Why changes them to Scala List? Just change this one to java.util.Collections.unmodifiableList(new ArrayList[(BlockId, BlockStatus)](_seq)) should be enough.

Thanks for the review @zsxwing . I checked the java doc and it says that getting iterator is not thread safe and suggests below usage. That's why I did _seq.synchronized

https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html

List list = Collections.synchronizedList(new ArrayList()); ... synchronized (list) { Iterator i = list.iterator(); // Must be in synchronized block while (i.hasNext()) foo(i.next()); }

Regarding your second questions, JsonProtocal is using it as Scala collection that is why I converted it to a Scala collection so we won't need to convert again.

Sorry. Didn't noticed this line (mutex = this;) in Collections.synchronizedList...

I just took a look at CollectionAccumulator. I think we can just make BlockStatusesAccumulator extends CollectionAccumulator. This would eliminate these duplicated codes.

Regarding #2, I take a look at CollectionAccumulator as well and it seems like a good idea. Let me give it a try.

SparkQA · 2016-10-07T21:43:15Z

Test build #66534 has finished for PR 15371 at commit 97977ee.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-07T21:47:13Z

Test build #66535 has finished for PR 15371 at commit 9572ebd.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-07T22:17:05Z

Test build #66537 has finished for PR 15371 at commit a5d3eb7.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

seyfe · 2016-10-07T22:25:05Z

Hi @zsxwing ,

The test failed with below error. I don't think that it's related with my change. Should we just re-run the test or do you have any suggestion?

java.lang.RuntimeException: spark-core: Binary compatibility check failed!
[error] (core/*:mimaReportBinaryIssues) spark-core: Binary compatibility check failed!

zsxwing · 2016-10-07T22:29:17Z

retest this please

SparkQA · 2016-10-07T22:50:27Z

Test build #66539 has finished for PR 15371 at commit a5d3eb7.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

seyfe · 2016-10-07T22:54:45Z

I don't know if it's related but I found a bug in the last iteration. We also need to override copy and copyAndReset methods. Otherwise it throws java.lang.ClassCastException error.

SparkQA · 2016-10-07T23:16:30Z

Test build #66541 has finished for PR 15371 at commit 2d5656c.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

seyfe · 2016-10-07T23:25:27Z

@zsxwing , I built it and the test pipelines works fine. So fix is good. But I don't know how to fix the MiMa tests. Would you mind helping me on this?

zsxwing · 2016-10-10T05:14:36Z

@seyfe I think we can remove BlockStatusesAccumulator and just use private val _updatedBlockStatuses = new CollectionAccumulator[(BlockId, BlockStatus)] instead. BlockStatusesAccumulator doesn't provide more functions than CollectionAccumulator. Sorry that I didn't find that early.

seyfe · 2016-10-10T05:29:37Z

@zsxwing , I think that is a good idea. I search it and that is the only place we use BlockStatusesAccumulator. Let me remove it.

SparkQA · 2016-10-10T06:48:52Z

Test build #66627 has finished for PR 15371 at commit da2311a.

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

zsxwing · 2016-10-10T06:50:53Z

retest this please

SparkQA · 2016-10-10T09:08:05Z

Test build #66632 has finished for PR 15371 at commit da2311a.

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

SparkQA · 2016-10-10T19:53:16Z

Test build #66669 has finished for PR 15371 at commit 5e00dc3.

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

zsxwing · 2016-10-11T03:41:09Z

LGTM. Thanks! Merging to master and ~~2.0~~.

zsxwing · 2016-10-11T03:43:05Z

There are some conflicts with 2.0. @seyfe could you submit a PR for branch-2.0, please? Thanks!

…kStatusesAccumulator Change the BlockStatusesAccumulator to return immutable object when value method is called. Existing tests plus I verified this change by running a pipeline which consistently repro this issue. This is the stack trace for this exception: ` java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901) at java.util.ArrayList$Itr.next(ArrayList.java:851) at scala.collection.convert.Wrappers$JIteratorWrapper.next(Wrappers.scala:43) at scala.collection.Iterator$class.foreach(Iterator.scala:893) at scala.collection.AbstractIterator.foreach(Iterator.scala:1336) at scala.collection.IterableLike$class.foreach(IterableLike.scala:72) at scala.collection.AbstractIterable.foreach(Iterable.scala:54) at scala.collection.generic.Growable$class.$plus$plus$eq(Growable.scala:59) at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:183) at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:45) at scala.collection.TraversableLike$class.to(TraversableLike.scala:590) at scala.collection.AbstractTraversable.to(Traversable.scala:104) at scala.collection.TraversableOnce$class.toList(TraversableOnce.scala:294) at scala.collection.AbstractTraversable.toList(Traversable.scala:104) at org.apache.spark.util.JsonProtocol$.accumValueToJson(JsonProtocol.scala:314) at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291) at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291) at scala.Option.map(Option.scala:146) at org.apache.spark.util.JsonProtocol$.accumulableInfoToJson(JsonProtocol.scala:291) at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283) at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234) at scala.collection.immutable.List.foreach(List.scala:381) at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:35) at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45) at scala.collection.TraversableLike$class.map(TraversableLike.scala:234) at scala.collection.AbstractTraversable.map(Traversable.scala:104) at org.apache.spark.util.JsonProtocol$.taskInfoToJson(JsonProtocol.scala:283) at org.apache.spark.util.JsonProtocol$.taskEndToJson(JsonProtocol.scala:145) at org.apache.spark.util.JsonProtocol$.sparkEventToJson(JsonProtocol.scala:76) ` Author: Ergin Seyfe <eseyfe@fb.com> Closes apache#15371 from seyfe/race_cond_jsonprotocal.

seyfe · 2016-10-11T04:19:07Z

Thanks @zsxwing.

Here is the PR for branch-2.0
#15425

…kStatusesAccumulator ## What changes were proposed in this pull request? Change the BlockStatusesAccumulator to return immutable object when value method is called. ## How was this patch tested? Existing tests plus I verified this change by running a pipeline which consistently repro this issue. This is the stack trace for this exception: ` java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901) at java.util.ArrayList$Itr.next(ArrayList.java:851) at scala.collection.convert.Wrappers$JIteratorWrapper.next(Wrappers.scala:43) at scala.collection.Iterator$class.foreach(Iterator.scala:893) at scala.collection.AbstractIterator.foreach(Iterator.scala:1336) at scala.collection.IterableLike$class.foreach(IterableLike.scala:72) at scala.collection.AbstractIterable.foreach(Iterable.scala:54) at scala.collection.generic.Growable$class.$plus$plus$eq(Growable.scala:59) at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:183) at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:45) at scala.collection.TraversableLike$class.to(TraversableLike.scala:590) at scala.collection.AbstractTraversable.to(Traversable.scala:104) at scala.collection.TraversableOnce$class.toList(TraversableOnce.scala:294) at scala.collection.AbstractTraversable.toList(Traversable.scala:104) at org.apache.spark.util.JsonProtocol$.accumValueToJson(JsonProtocol.scala:314) at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291) at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291) at scala.Option.map(Option.scala:146) at org.apache.spark.util.JsonProtocol$.accumulableInfoToJson(JsonProtocol.scala:291) at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283) at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234) at scala.collection.immutable.List.foreach(List.scala:381) at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:35) at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45) at scala.collection.TraversableLike$class.map(TraversableLike.scala:234) at scala.collection.AbstractTraversable.map(Traversable.scala:104) at org.apache.spark.util.JsonProtocol$.taskInfoToJson(JsonProtocol.scala:283) at org.apache.spark.util.JsonProtocol$.taskEndToJson(JsonProtocol.scala:145) at org.apache.spark.util.JsonProtocol$.sparkEventToJson(JsonProtocol.scala:76) ` Author: Ergin Seyfe <eseyfe@fb.com> Closes apache#15371 from seyfe/race_cond_jsonprotocal.

seyfe changed the title ~~[SPARK-17463] [Core] TaskInfoToJson should map using List(immutable) rather than ListBuffer(mutable)~~ [SPARK-17463] [Core] Fix ConcurrentModificationException issue in JsonProtocol Oct 6, 2016

srowen requested changes Oct 6, 2016

View reviewed changes

seyfe changed the title ~~[SPARK-17463] [Core] Fix ConcurrentModificationException issue in JsonProtocol~~ [SPARK-17816] [Core] Fix ConcurrentModificationException issue in JsonProtocol Oct 6, 2016

srowen reviewed Oct 7, 2016

View reviewed changes

zsxwing reviewed Oct 7, 2016

View reviewed changes

seyfe added 15 commits October 10, 2016 10:27

Map for List rather than ListBuffer

a150679

Synchronize block accumulator

7333adb

Added back empty lines that separates the impor statements

8382ed1

Synchronized asScala calls which use iterator

ba163f2

BlockStatusesAccumulator should extend from CollectionAccumulator

f833dc9

revert the change in JsonProtocolSuite

32af98f

remove the unused namespace java.util.Collections

9907345

Fix type erasure compiler warning

4491402

Implement copy and copyAndReset methods for BlockStatusesAccumulator

d392f91

Fix MiMa tests by excluding AccumulatorV2 methods

c89f23c

Moved MimaExcludes to v2.1 section

20f9762

Use merge() method when copying

f87450a

Use synchronized block rather that merge method

35313c6

Remove BlockStatusesAccumulator

4bca3bd

Try removing MimaExcludes

5e00dc3

seyfe force-pushed the race_cond_jsonprotocal branch from da2311a to 5e00dc3 Compare October 10, 2016 17:29

asfgit closed this in 19a5bae Oct 11, 2016

seyfe deleted the race_cond_jsonprotocal branch February 27, 2017 19:36

Uh oh!

Conversation

seyfe commented Oct 6, 2016 • 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!

hvanhovell commented Oct 6, 2016

Uh oh!

SparkQA commented Oct 6, 2016

Uh oh!

seyfe commented Oct 6, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Oct 6, 2016

Uh oh!

zsxwing commented Oct 6, 2016

Uh oh!

seyfe commented Oct 6, 2016

Uh oh!

SparkQA commented Oct 7, 2016

Uh oh!

zsxwing commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

zsxwing commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

srowen commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

zsxwing commented Oct 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 7, 2016

Uh oh!

SparkQA commented Oct 7, 2016

Uh oh!

SparkQA commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

zsxwing commented Oct 7, 2016

Uh oh!

SparkQA commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

Uh oh!

SparkQA commented Oct 7, 2016

Uh oh!

seyfe commented Oct 7, 2016

seyfe commented Oct 6, 2016 •

edited

Loading

zsxwing commented Oct 11, 2016 •

edited

Loading