Skip to content

Commit 0732e4f

Browse files
committed
[SC-5116] Fix race conditions for compiling codes and loading classes in Scala 2.10 REPL
## What changes were proposed in this pull request? Mostly same as https://github.com/databricks/universe/pull/13752. This can be directly fixed in databricks/spark for Scala 2.10 because Spark Scala 2.10 forks the REPL codes. ## How was this patch tested? build/sbt -Dscala-2.10 -Pscala-2.10 "repl/test-only *ReplSuite" Author: Shixiong Zhu <[email protected]> Closes apache#128 from zsxwing/fix-scala210-repl-crash.
1 parent a918d53 commit 0732e4f

File tree

3 files changed

+33
-5
lines changed

3 files changed

+33
-5
lines changed

repl/scala-2.10/src/main/scala/org/apache/spark/repl/SparkIMain.scala

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,26 @@ import org.apache.spark.annotation.DeveloperApi
8585
*
8686
* @author Moez A. Abdel-Gawad
8787
* @author Lex Spoon
88+
*
89+
* Comments for Databricks changes:
90+
*
91+
* `SparkIMain.TranslatingClassLoader` is not thread-safe because it accesses non-thread-safe
92+
* fields of `SparkIMain`. (See https://databricks.atlassian.net/browse/SC-5006 and
93+
* https://issues.scala-lang.org/browse/SI-10045)
94+
* So we need to add necessary locks to make sure no class is being loaded when compiling codes.
95+
*
96+
* We use `SparkIMain.classLoader` (an instance of `SparkIMain.TranslatingClassLoader`)
97+
* to lock is because methods in `TranslatingClassLoader` has already been protected by `TranslatingClassLoader`.
98+
* If we use other locks, it's easy to cause dead-lock. Therefore, we just use `SparkIMain.classLoader`.
99+
*
100+
* The following 3 places will be called by us and they need to be protected:
101+
*
102+
* - Compile sources. `requestFromLine` is override to add the lock.
103+
* - Code completation. See `SparkJLineCompletion.JLineTabCompletion`.
104+
* - Compile a package cell. `compileSources` is override to add the lock.
105+
*
106+
* The fix for Scala 2.11 is in databricks/universe:
107+
* https://github.com/databricks/universe/commit/034d8a7c16e81d74fe655c7ed7cbd2aad6421b15
88108
*/
89109
@DeveloperApi
90110
class SparkIMain(
@@ -654,8 +674,9 @@ import org.apache.spark.annotation.DeveloperApi
654674
* @return True if successful, otherwise false
655675
*/
656676
@DeveloperApi
657-
def compileSources(sources: SourceFile*): Boolean =
677+
def compileSources(sources: SourceFile*): Boolean = classLoader.synchronized {
658678
compileSourcesKeepingRun(sources: _*)._1
679+
}
659680

660681
/**
661682
* Compiles a string of code.
@@ -702,7 +723,7 @@ import org.apache.spark.annotation.DeveloperApi
702723
}
703724

704725

705-
private def requestFromLine(line: String, synthetic: Boolean): Either[IR.Result, Request] = {
726+
private def requestFromLine(line: String, synthetic: Boolean): Either[IR.Result, Request] = classLoader.synchronized {
706727
val content = indentCode(line)
707728
val trees = parse(content) match {
708729
case None => return Left(IR.Incomplete)
@@ -849,7 +870,10 @@ import org.apache.spark.annotation.DeveloperApi
849870
case Right(req) =>
850871
// null indicates a disallowed statement type; otherwise compile and
851872
// fail if false (implying e.g. a type error)
852-
if (req == null || !req.compile) IR.Error
873+
val hasError = classLoader.synchronized {
874+
req == null || !req.compile
875+
}
876+
if (hasError) IR.Error
853877
else loadAndRunReq(req)
854878
}
855879
}

repl/scala-2.10/src/main/scala/org/apache/spark/repl/SparkJLineCompletion.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ class SparkJLineCompletion(val intp: SparkIMain) extends Completion with Complet
343343
}
344344

345345
// This is jline's entry point for completion.
346-
override def complete(buf: String, cursor: Int): Candidates = {
346+
override def complete(buf: String, cursor: Int): Candidates = intp.classLoader.synchronized {
347347
verbosity = if (isConsecutiveTabs(buf, cursor)) verbosity + 1 else 0
348348
logDebug("\ncomplete(%s, %d) last = (%s, %d), verbosity: %s".format(buf, cursor, lastBuf, lastCursor, verbosity))
349349

repl/scala-2.10/src/test/scala/org/apache/spark/repl/ReplSuite.scala

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,11 @@ class ReplSuite extends SparkFunSuite {
292292
assertDoesNotContain("Exception", output)
293293
}
294294

295-
test("SPARK-2632 importing a method from non serializable class and not using it.") {
295+
// scalastyle:off
296+
// Known failure because of https://github.com/databricks/spark/commit/e46b87a1b9e23f53d12c32a2580ed2f6c61aefc3
297+
// The patch allows people to define a class and use implicit imports in the same cell.
298+
// scalastyle:on
299+
ignore("SPARK-2632 importing a method from non serializable class and not using it.") {
296300
val output = runInterpreter("local-cluster[1,1,1024]",
297301
"""
298302
|class TestClass() { def testMethod = 3 }

0 commit comments

Comments
 (0)