SI-9126 Missing .seqs causes problems with parallel GenXs

Ichoran · Ichoran · commit 1d4a1f721d36 · 2015-02-20T00:07:44.000-08:00
Added `.seq` in two essential places so that a parallel collection passed as an argument won't mess up side-effecting sequential computations in `List.flatMap` and `GenericTraversableTemplate.transpose` (thanks to retronym for finding the danger spots).

Tests that `.seq` is called by constructing a `GenSeq` whose `.seq` disagrees with anything non-`.seq` (idea &amp; working implementation from retronym).

Also updates the `.seq` test for `Vector#++` to use the new more efficient method.
diff --git a/src/library/scala/collection/generic/GenericTraversableTemplate.scala b/src/library/scala/collection/generic/GenericTraversableTemplate.scala
@@ -216,7 +216,7 @@ trait GenericTraversableTemplate[+A, +CC[X] <: GenTraversable[X]] extends HasNew
     val bs: IndexedSeq[Builder[B, CC[B]]] = IndexedSeq.fill(headSize)(genericBuilder[B])
     for (xs <- sequential) {
       var i = 0
-      for (x <- asTraversable(xs)) {
+      for (x <- asTraversable(xs).seq) {
         if (i >= headSize) fail
         bs(i) += x
         i += 1
diff --git a/src/library/scala/collection/immutable/List.scala b/src/library/scala/collection/immutable/List.scala
@@ -324,7 +324,7 @@ sealed abstract class List[+A] extends AbstractSeq[A]
         var h: ::[B] = null
         var t: ::[B] = null
         while (rest ne Nil) {
-          f(rest.head).foreach{ b =>
+          f(rest.head).seq.foreach{ b =>
             if (!found) {
               h = new ::(b, Nil)
               t = h
diff --git a/test/junit/scala/collection/ParallelConsistencyTest.scala b/test/junit/scala/collection/ParallelConsistencyTest.scala
@@ -0,0 +1,44 @@
+package scala.collection.immutable
+
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+
+@RunWith(classOf[JUnit4])
+class ParallelConsistencyTest {
+
+  private val theSeq = Seq(1,2,3)
+
+  // This collection will throw an exception if you do anything but call .length or .seq
+  private val mustCallSeq: collection.GenSeq[Int] = new collection.parallel.ParSeq[Int] {
+    def length = 3
+    
+    // This method is surely sequential & safe -- want all access to go through here
+    def seq = theSeq
+    
+    def notSeq = throw new Exception("Access to parallel collection not via .seq")
+    
+    // These methods could possibly be used dangerously explicitly or internally
+    // (apply could also be used safely; if it is, do test with mustCallSeq)
+    def apply(i: Int) = notSeq
+    def splitter = notSeq
+  }  
+
+   // Test Vector ++ with a small parallel collection concatenation (SI-9072).
+  @Test
+  def testPlusPlus(): Unit = {
+    assert((Vector.empty ++ mustCallSeq) == theSeq, "Vector ++ unsafe with parallel vectors")
+  }
+  
+  // SI-9126, 1 of 2
+  @Test
+  def testTranspose(): Unit = {
+    assert(List(mustCallSeq).transpose.flatten == theSeq, "Transposing inner parallel collection unsafe")
+  }
+  
+  // SI-9126, 2 of 2
+  @Test
+  def testList_flatMap(): Unit = {
+    assert(List(1).flatMap(_ => mustCallSeq) == theSeq, "List#flatMap on inner parallel collection unsafe")
+  }
+}
diff --git a/test/junit/scala/collection/immutable/VectorTest.scala b/test/junit/scala/collection/immutable/VectorTest.scala
