New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in non-forked, parallel tests. #3985

Closed
wants to merge 1 commit into
base: 1.1.x
from

Conversation

Projects
None yet
3 participants
@retronym
Member

retronym commented Mar 5, 2018

Non forked tests that are run in parallel groups can call into
a single instance of TestStatusReporter concurrently.

This seems to be limited to the startGroup/endGroup/testEvent
methods called in:

def runTest() = {
// here we get the results! here is where we'd pass in the event listener
val results = new scala.collection.mutable.ListBuffer[Event]
val handler = new EventHandler { def handle(e: Event): Unit = { results += e } }
val loggers: Vector[ContentLogger] = listeners.flatMap(_.contentLogger(testDefinition))
val nestedTasks =
try testTask.execute(handler, loggers.map(_.log).toArray)
finally {
loggers.foreach(_.flush())
}
val event = TestEvent(results)
safeListenersCall(_.testEvent(event))
(SuiteResult(results), nestedTasks.toSeq)
}
safeListenersCall(_.startGroup(name))
try {
val (suiteResult, nestedTasks) = runTest()
safeListenersCall(_.endGroup(name, suiteResult.result))
(suiteResult, nestedTasks)
} catch {
case NonFatal(e) =>
safeListenersCall(_.endGroup(name, e))
(SuiteResult.Error, Seq.empty[TestTask])
}

Which itself is called within:

private def createTestTasks(loader: ClassLoader,
runners: Map[Framework, TestRunner],
tests: Map[Framework, Set[TestDefinition]],
ordered: Vector[TestDefinition],
log: ManagedLogger,
listeners: Vector[TestReportListener])
: (() => Unit, Vector[(String, TestFunction)], TestResult => (() => Unit)) = {
val testsListeners = listeners collect { case tl: TestsListener => tl }
def foreachListenerSafe(f: TestsListener => Unit): () => Unit =
() => safeForeach(testsListeners, log)(f)
val startTask = foreachListenerSafe(_.doInit)
val testTasks =
tests flatMap {
case (framework, testDefinitions) =>
val runner = runners(framework)
val testTasks = withContextLoader(loader) { runner.tasks(testDefinitions) }
for (testTask <- testTasks) yield {
val taskDef = testTask.taskDef
(taskDef.fullyQualifiedName, createTestFunction(loader, taskDef, runner, testTask))
}
}
val endTask = (result: TestResult) => foreachListenerSafe(_.doComplete(result))
(startTask, order(testTasks, ordered), endTask)
}

Creating the runnables that are run in parallel (in builds so configured):

val (frameworkSetup, runnables, frameworkCleanup) =
TestFramework.testTasks(frameworks, runners, loader, tests, log, testListeners)
val setupTasks = fj(partApp(userSetup) :+ frameworkSetup)
val mainTasks =
if (config.parallel)
makeParallel(loader, runnables, setupTasks, config.tags) //.toSeq.join
else
makeSerial(loader, runnables, setupTasks, config.tags)

We believe this to be the cause of the hang witnessed in
the a suite of Scalacheck-framework tests in the Scala
build:

scala/scala-jenkins-infra#249

This commit uses a concurrent map to support concurrent
status updates.

@eed3si9n eed3si9n added the in progress label Mar 5, 2018

@eed3si9n

Thanks!
A minor comment.

@@ -32,13 +34,17 @@ private[sbt] class TestStatusReporter(f: File) extends TestsListener {
private[sbt] object TestStatus {
import java.util.Properties
def read(f: File): Map[String, Long] = {
def read(f: File): concurrent.Map[String, Long] = {
import scala.collection.JavaConverters._
val properties = new Properties
IO.load(properties, f)
properties.asScala map { case (k, v) => (k, v.toLong) }

This comment has been minimized.

@eed3si9n

eed3si9n Mar 5, 2018

Member

This line should be removed?

This comment has been minimized.

@retronym

retronym Mar 5, 2018

Member

yep, push-fed

Fix race condition in non-forked, parallel tests.
Non forked tests that are run in parallel groups can call into
a single instance of TestStatusReporter concurrently.

This seems to be limited to the startGroup/endGroup/testEvent
methods called in:

  https://github.com/sbt/sbt/blob/a41727fb17bb923036f90ca2ca62897def1a893e/testing/src/main/scala/sbt/TestFramework.scala#L100-L124

Which itself is called within:

  https://github.com/sbt/sbt/blob/a41727fb17bb923036f90ca2ca62897def1a893e/testing/src/main/scala/sbt/TestFramework.scala#L203-L229

Creating the `runnables` that are run in parallel (in builds so configured):

  https://github.com/sbt/sbt/blob/a6eb1260c8162bfbfcbfb8656f152854a93d33ae/main-actions/src/main/scala/sbt/Tests.scala#L222-L230

We believe this to be the cause of the hang witnessed in
the a suite of Scalacheck-framework tests in the Scala
build:

  scala/scala-jenkins-infra#249

This commit uses a concurrent map to support concurrent
status updates.

retronym added a commit to retronym/scala that referenced this pull request Mar 5, 2018

Disable parallelism in the scalacheck suite
This is a workaround for a race condition we identified:

  scala/scala-jenkins-infra#249

A future SBT version will include this fix:

  sbt/sbt#3985
@dwijnand

lgtm, but you need to tell MiMa to chillax on the private[sbt] changes:

[error]  * method write(scala.collection.mutable.Map,java.lang.String,java.io.File)Unit in object sbt.TestStatus's type is different in current version, where it is (scala.collection.Map,java.lang.String,java.io.File)Unit instead of (scala.collection.mutable.Map,java.lang.String,java.io.File)Unit
[error]    filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("sbt.TestStatus.write")
[error]  * method read(java.io.File)scala.collection.mutable.Map in object sbt.TestStatus has a different result type in current version, where it is scala.collection.concurrent.Map rather than scala.collection.mutable.Map
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("sbt.TestStatus.read")
@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Mar 7, 2018

This is now merged as #3997

@eed3si9n eed3si9n closed this Mar 7, 2018

@eed3si9n eed3si9n removed the in progress label Mar 7, 2018

@dwijnand dwijnand added this to the 1.1.2 milestone Mar 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment