From bf20737faa2da5b45ad1ef5e6a43dff307c99788 Mon Sep 17 00:00:00 2001 From: Viktor Klang Date: Sat, 20 Dec 2014 19:51:43 +0100 Subject: SI-8689 Avoid internal error in Promise after sequence of completions Calling `completeWith` when the `DefaultPromise` is already completed, leads to callbacks not being properly executed. This happened because `Future.InternalCallbackExecutor` extends `BatchingExecutor`[1] which assumes `unbatchedExecute` to be async, when in this case it is sync, and if there is an exception thrown by executing the batch, it creates a new batch with the remaining items from the current batch and submits that to `unbatchedExecute` and then rethrows, but if you have a sync `unbatchedExecute`, it will fail since it is not reentrant, as witnessed by the failed `require` as reported in this issue. This commit avoids problem by delegating `completeWith` to `tryComplete`, which has the effect of using `onComplete` + `tryComplete` i.s.o. `complete`, which means that when it fails (because of a benign race condition between completers) it won't throw an exception. It has been tested by the minimized reproducer. [1] Actually, in the 2.10.x branch where this patch is starting out, "The BatchingExecutor trait had to be inlined into InternalCallbackExecutor for binary compatibility.". This comment will be more literally correct in the context of 2.11.x and beyond --- test/files/jvm/future-spec/PromiseTests.scala | 77 +++++++++++++++++++++++---- test/files/jvm/t8689.check | 1 + test/files/jvm/t8689.scala | 13 +++++ 3 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 test/files/jvm/t8689.check create mode 100644 test/files/jvm/t8689.scala (limited to 'test') diff --git a/test/files/jvm/future-spec/PromiseTests.scala b/test/files/jvm/future-spec/PromiseTests.scala index 48f94666ba..3b20a96502 100644 --- a/test/files/jvm/future-spec/PromiseTests.scala +++ b/test/files/jvm/future-spec/PromiseTests.scala @@ -43,21 +43,80 @@ object PromiseTests extends MinimalScalaTest { Await.result(failure fallbackTo otherFailure, defaultTimeout) }.getMessage mustBe ("br0ken") } - + + "be completable with a completed Promise" in { + { + val p = Promise[String]() + p.tryCompleteWith(Promise[String]().success("foo").future) + Await.result(p.future, defaultTimeout) mustBe ("foo") + } + { + val p = Promise[String]() + p.completeWith(Promise[String]().success("foo").future) + Await.result(p.future, defaultTimeout) mustBe ("foo") + } + { + val p = Promise[String]() + p.tryCompleteWith(Promise[String]().failure(new RuntimeException("br0ken")).future) + intercept[RuntimeException] { + Await.result(p.future, defaultTimeout) + }.getMessage mustBe ("br0ken") + } + { + val p = Promise[String]() + p.tryCompleteWith(Promise[String]().failure(new RuntimeException("br0ken")).future) + intercept[RuntimeException] { + Await.result(p.future, defaultTimeout) + }.getMessage mustBe ("br0ken") + } + } } "A successful Promise" should { - val result = "test value" - val promise = Promise[String]().complete(Success(result)) - promise.isCompleted mustBe (true) - futureWithResult(_(promise.future, result)) + "be completed" in { + val result = "test value" + val promise = Promise[String]().complete(Success(result)) + promise.isCompleted mustBe (true) + futureWithResult(_(promise.future, result)) + } + + "not be completable with a completed Promise" in { + { + val p = Promise.successful("bar") + p.tryCompleteWith(Promise[String]().success("foo").future) + Await.result(p.future, defaultTimeout) mustBe ("bar") + } + { + val p = Promise.successful("bar") + p.completeWith(Promise[String]().success("foo").future) + Await.result(p.future, defaultTimeout) mustBe ("bar") + } + } } "A failed Promise" should { - val message = "Expected Exception" - val promise = Promise[String]().complete(Failure(new RuntimeException(message))) - promise.isCompleted mustBe (true) - futureWithException[RuntimeException](_(promise.future, message)) + "be completed" in { + val message = "Expected Exception" + val promise = Promise[String]().complete(Failure(new RuntimeException(message))) + promise.isCompleted mustBe (true) + futureWithException[RuntimeException](_(promise.future, message)) + } + "not be completable with a completed Promise" in { + { + val p = Promise[String]().failure(new RuntimeException("unbr0ken")) + p.tryCompleteWith(Promise[String].failure(new Exception("br0ken")).future) + intercept[RuntimeException] { + Await.result(p.future, defaultTimeout) + }.getMessage mustBe ("unbr0ken") + } + { + val p = Promise[String]().failure(new RuntimeException("unbr0ken")) + p.completeWith(Promise[String]().failure(new Exception("br0ken")).future) + intercept[RuntimeException] { + Await.result(p.future, defaultTimeout) + }.getMessage mustBe ("unbr0ken") + } + } } "An interrupted Promise" should { diff --git a/test/files/jvm/t8689.check b/test/files/jvm/t8689.check new file mode 100644 index 0000000000..2e9ba477f8 --- /dev/null +++ b/test/files/jvm/t8689.check @@ -0,0 +1 @@ +success diff --git a/test/files/jvm/t8689.scala b/test/files/jvm/t8689.scala new file mode 100644 index 0000000000..ef43a1df63 --- /dev/null +++ b/test/files/jvm/t8689.scala @@ -0,0 +1,13 @@ +object Test { + def main(args: Array[String]): Unit = { + import scala.concurrent._ + import ExecutionContext.Implicits.global + val source1 = Promise[Int]() + val source2 = Promise[Int]() + source2.completeWith(source1.future).future.onComplete { + case _ => print("success") + } + source2.tryFailure(new TimeoutException) + source1.success(123) + } +} \ No newline at end of file -- cgit v1.2.3 From 2f5ff595fd141025de30dadfc97870ef01d44c9f Mon Sep 17 00:00:00 2001 From: Miles Sabin Date: Mon, 9 Feb 2015 21:25:26 +0000 Subject: Backported fix for SI-7753 to 2.10.x. --- src/reflect/scala/reflect/internal/Types.scala | 89 ++++++++++++++++---------- test/files/neg/t3873.check | 4 +- test/files/neg/t3873.scala | 2 +- test/files/pos/t7753.scala | 36 +++++++++++ test/files/pos/t8223.scala | 29 +++++++++ 5 files changed, 123 insertions(+), 37 deletions(-) create mode 100644 test/files/pos/t7753.scala create mode 100644 test/files/pos/t8223.scala (limited to 'test') diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala index 2f49995030..15f62bca30 100644 --- a/src/reflect/scala/reflect/internal/Types.scala +++ b/src/reflect/scala/reflect/internal/Types.scala @@ -4818,37 +4818,30 @@ trait Types extends api.Types { self: SymbolTable => } /** Note: This map is needed even for non-dependent method types, despite what the name might imply. - */ + */ class InstantiateDependentMap(params: List[Symbol], actuals0: List[Type]) extends TypeMap with KeepOnlyTypeConstraints { private val actuals = actuals0.toIndexedSeq private val existentials = new Array[Symbol](actuals.size) - def existentialsNeeded: List[Symbol] = existentials.filter(_ ne null).toList - - private object StableArg { - def unapply(param: Symbol) = Arg unapply param map actuals filter (tp => - tp.isStable && (tp.typeSymbol != NothingClass) - ) - } - private object Arg { - def unapply(param: Symbol) = Some(params indexOf param) filter (_ >= 0) - } - - def apply(tp: Type): Type = mapOver(tp) match { - // unsound to replace args by unstable actual #3873 - case SingleType(NoPrefix, StableArg(arg)) => arg - // (soundly) expand type alias selections on implicit arguments, - // see depmet_implicit_oopsla* test cases -- typically, `param.isImplicit` - case tp1 @ TypeRef(SingleType(NoPrefix, Arg(pid)), sym, targs) => - val arg = actuals(pid) - val res = typeRef(arg, sym, targs) - if (res.typeSymbolDirect.isAliasType) res.dealias else tp1 - // don't return the original `tp`, which may be different from `tp1`, - // due to dropping annotations - case tp1 => tp1 + def existentialsNeeded: List[Symbol] = existentials.iterator.filter(_ ne null).toList + + private object StableArgTp { + // type of actual arg corresponding to param -- if the type is stable + def unapply(param: Symbol): Option[Type] = (params indexOf param) match { + case -1 => None + case pid => + val tp = actuals(pid) + if (tp.isStable && (tp.typeSymbol != NothingClass)) Some(tp) + else None + } } - /* Return the type symbol for referencing a parameter inside the existential quantifier. - * (Only needed if the actual is unstable.) + /** Return the type symbol for referencing a parameter that's instantiated to an unstable actual argument. + * + * To soundly abstract over an unstable value (x: T) while retaining the most type information, + * use `x.type forSome { type x.type <: T with Singleton}` + * `typeOf[T].narrowExistentially(symbolOf[x])`. + * + * See also: captureThis in AsSeenFromMap. */ private def existentialFor(pid: Int) = { if (existentials(pid) eq null) { @@ -4856,11 +4849,43 @@ trait Types extends api.Types { self: SymbolTable => existentials(pid) = ( param.owner.newExistential(param.name.toTypeName append nme.SINGLETON_SUFFIX, param.pos, param.flags) setInfo singletonBounds(actuals(pid)) - ) + ) } existentials(pid) } + private object UnstableArgTp { + // existential quantifier and type of corresponding actual arg with unstable type + def unapply(param: Symbol): Option[(Symbol, Type)] = (params indexOf param) match { + case -1 => None + case pid => + val sym = existentialFor(pid) + Some((sym, sym.tpe)) // refers to an actual value, must be kind-* + } + } + + private object StabilizedArgTp { + def unapply(param: Symbol): Option[Type] = + param match { + case StableArgTp(tp) => Some(tp) // (1) + case UnstableArgTp(_, tp) => Some(tp) // (2) + case _ => None + } + } + + /** instantiate `param.type` to the (sound approximation of the) type `T` + * of the actual argument `arg` that was passed in for `param` + * + * (1) If `T` is stable, we can just use that. + * + * (2) SI-3873: it'd be unsound to instantiate `param.type` to an unstable `T`, + * so we approximate to `X forSome {type X <: T with Singleton}` -- we can't soundly say more. + */ + def apply(tp: Type): Type = tp match { + case SingleType(NoPrefix, StabilizedArgTp(tp)) => tp + case _ => mapOver(tp) + } + //AM propagate more info to annotations -- this seems a bit ad-hoc... (based on code by spoon) override def mapOver(arg: Tree, giveup: ()=>Nothing): Tree = { // TODO: this should be simplified; in the stable case, one can @@ -4883,13 +4908,9 @@ trait Types extends api.Types { self: SymbolTable => // Both examples are from run/constrained-types.scala. object treeTrans extends Transformer { override def transform(tree: Tree): Tree = tree.symbol match { - case StableArg(actual) => - gen.mkAttributedQualifier(actual, tree.symbol) - case Arg(pid) => - val sym = existentialFor(pid) - Ident(sym) copyAttrs tree setType typeRef(NoPrefix, sym, Nil) - case _ => - super.transform(tree) + case StableArgTp(tp) => gen.mkAttributedQualifier(tp, tree.symbol) + case UnstableArgTp(quant, tp) => Ident(quant) copyAttrs tree setType tp + case _ => super.transform(tree) } } treeTrans transform arg diff --git a/test/files/neg/t3873.check b/test/files/neg/t3873.check index 54d6abdf63..f9f413aeaf 100644 --- a/test/files/neg/t3873.check +++ b/test/files/neg/t3873.check @@ -1,6 +1,6 @@ t3873.scala:11: error: type mismatch; found : Test.a.B - required: a.B - wrongf(new A)(a.b) // should not compile -- TODO: improve error message? the "a" is ambiguous + required: a.B where val a: A + wrongf(new A)(a.b) // should not compile ^ one error found diff --git a/test/files/neg/t3873.scala b/test/files/neg/t3873.scala index e7815f0937..b27b4e9c9d 100644 --- a/test/files/neg/t3873.scala +++ b/test/files/neg/t3873.scala @@ -8,5 +8,5 @@ object Test { val a = new A wrongf(a)(a.b) - wrongf(new A)(a.b) // should not compile -- TODO: improve error message? the "a" is ambiguous + wrongf(new A)(a.b) // should not compile } \ No newline at end of file diff --git a/test/files/pos/t7753.scala b/test/files/pos/t7753.scala new file mode 100644 index 0000000000..93ad23f114 --- /dev/null +++ b/test/files/pos/t7753.scala @@ -0,0 +1,36 @@ +import scala.language.{ higherKinds, implicitConversions } + +trait Foo { type Out } + +trait SI { + val instance: Foo + type Out +} + +object Test { + def test { + def indirect(si: SI)(v: si.instance.Out) = v + + val foo: Foo { type Out = Int } = ??? + def conv(i: Foo): SI { type Out = i.Out; val instance: i.type } = ??? + + val converted = conv(foo) + + val v1: Int = indirect(converted)(23) // Okay (after refining the return type `instance` in the return type of `conv`) + /* + indirect(converted){(v: converted.instance.Out)converted.instance.Out}( + 23{Int(23)} + ){converted.instance.Out}; + */ + + val v2: Int = indirect(conv(foo))(23) // Used to fail as follows: + /* + indirect( + conv(foo){si.SI{type Out = foo.Out; val instance: si.Test..type}} + ){(v: si.instance.Out)si.instance.Out}( + 23{} + ){}; + */ + + } +} diff --git a/test/files/pos/t8223.scala b/test/files/pos/t8223.scala new file mode 100644 index 0000000000..52d6b0098e --- /dev/null +++ b/test/files/pos/t8223.scala @@ -0,0 +1,29 @@ +package p { + class ViewEnv[AIn] { + type A = AIn + class SubView { def has(x: A): Boolean = ??? } + def get: SubView = new SubView + } + + trait HasA { type A } + trait Indexable[R] extends HasA + class ArrayTC[AIn] extends Indexable[Array[AIn]] { type A = AIn } +} + +package object p { + implicit def arrayTypeClass[A] : ArrayTC[A] = new ArrayTC[A] + object intArrayTC extends ArrayTC[Int] + + type EnvAlias[W <: HasA] = ViewEnv[W#A] + type SubAlias[W <: HasA] = ViewEnv[W#A]#SubView + + def f0[R](xs: R)(implicit tc: Indexable[R]): ViewEnv[tc.A]#SubView = new ViewEnv[tc.A]() get + def f1[R](xs: R)(implicit tc: Indexable[R]): EnvAlias[tc.type]#SubView = new ViewEnv[tc.A]() get + def f2[R](xs: R)(implicit tc: Indexable[R]): SubAlias[tc.type] = new ViewEnv[tc.A]() get + + def g0 = f0(Array(1)) has 2 // ok + def g1 = f1(Array(1)) has 2 // ok + def g2 = f2(Array(1)) has 2 // "found: Int(2), required: tc.A" + def g3 = f2(Array(1))(new ArrayTC[Int]) has 2 // "found: Int(2), required: tc.A" + def g4 = f2(Array(1))(intArrayTC) has 2 // ok +} -- cgit v1.2.3