diff options
author | Eugene Burmako <xeno.by@gmail.com> | 2014-01-15 21:07:15 +0300 |
---|---|---|
committer | Eugene Burmako <xeno.by@gmail.com> | 2014-01-21 14:12:39 +0300 |
commit | 2bd304404ac00939a18a678aa982da9cbb3471a2 (patch) | |
tree | 3cbd2f12522953a0cd9945aca7fae02f44ec1b94 | |
parent | f142d854d3401728546ae6822662b7a3ad655a58 (diff) | |
download | scala-2bd304404ac00939a18a678aa982da9cbb3471a2.tar.gz scala-2bd304404ac00939a18a678aa982da9cbb3471a2.tar.bz2 scala-2bd304404ac00939a18a678aa982da9cbb3471a2.zip |
SI-8131 fixes residual race condition in runtime reflection
Apparently some completers can call setInfo while they’re not yet done,
which resets the LOCKED flag, and makes anything that uses LOCKED to
track completion unreliable. Unfortunately, that’s exactly the mechanism
that was used by runtime reflection to elide locking for symbols that are
known to be initialized.
This commit fixes the problematic lock elision strategy by introducing
an explicit communication channel between SynchronizedSymbol’s and their
completers. Now instead of trying hard to infer whether it’s already
initialized or not, every symbol gets a volatile field that can be queried
to provide necessary information.
-rw-r--r-- | src/reflect/scala/reflect/internal/BuildUtils.scala | 8 | ||||
-rw-r--r-- | src/reflect/scala/reflect/internal/Definitions.scala | 19 | ||||
-rw-r--r-- | src/reflect/scala/reflect/internal/Importers.scala | 3 | ||||
-rw-r--r-- | src/reflect/scala/reflect/internal/Symbols.scala | 138 | ||||
-rw-r--r-- | src/reflect/scala/reflect/internal/pickling/UnPickler.scala | 12 | ||||
-rw-r--r-- | src/reflect/scala/reflect/runtime/JavaMirrors.scala | 26 | ||||
-rw-r--r-- | src/reflect/scala/reflect/runtime/JavaUniverseForce.scala | 1 | ||||
-rw-r--r-- | src/reflect/scala/reflect/runtime/SymbolLoaders.scala | 5 | ||||
-rw-r--r-- | src/reflect/scala/reflect/runtime/SymbolTable.scala | 15 | ||||
-rw-r--r-- | src/reflect/scala/reflect/runtime/SynchronizedSymbols.scala | 115 | ||||
-rw-r--r-- | test/files/run/reflection-sync-potpourri.scala (renamed from test/pending/run/reflection-sync-potpourri.scala) | 0 |
11 files changed, 224 insertions, 118 deletions
diff --git a/src/reflect/scala/reflect/internal/BuildUtils.scala b/src/reflect/scala/reflect/internal/BuildUtils.scala index 0a81bfa2a5..eb1777721e 100644 --- a/src/reflect/scala/reflect/internal/BuildUtils.scala +++ b/src/reflect/scala/reflect/internal/BuildUtils.scala @@ -33,19 +33,19 @@ trait BuildUtils { self: SymbolTable => } def newFreeTerm(name: String, value: => Any, flags: Long = 0L, origin: String = null): FreeTermSymbol = - newFreeTermSymbol(newTermName(name), value, flags, origin) + newFreeTermSymbol(newTermName(name), value, flags, origin).markFlagsCompleted(mask = AllFlags) def newFreeType(name: String, flags: Long = 0L, origin: String = null): FreeTypeSymbol = - newFreeTypeSymbol(newTypeName(name), flags, origin) + newFreeTypeSymbol(newTypeName(name), flags, origin).markFlagsCompleted(mask = AllFlags) def newNestedSymbol(owner: Symbol, name: Name, pos: Position, flags: Long, isClass: Boolean): Symbol = - owner.newNestedSymbol(name, pos, flags, isClass) + owner.newNestedSymbol(name, pos, flags, isClass).markFlagsCompleted(mask = AllFlags) def setAnnotations[S <: Symbol](sym: S, annots: List[AnnotationInfo]): S = sym.setAnnotations(annots) def setTypeSignature[S <: Symbol](sym: S, tpe: Type): S = - sym.setTypeSignature(tpe) + sym.setTypeSignature(tpe).markAllCompleted() def This(sym: Symbol): Tree = self.This(sym) diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala index 0091f50fc6..9c9e9aa87a 100644 --- a/src/reflect/scala/reflect/internal/Definitions.scala +++ b/src/reflect/scala/reflect/internal/Definitions.scala @@ -30,12 +30,12 @@ trait Definitions extends api.StandardDefinitions { private def enterNewClass(owner: Symbol, name: TypeName, parents: List[Type], flags: Long = 0L): ClassSymbol = { val clazz = owner.newClassSymbol(name, NoPosition, flags) - clazz setInfoAndEnter ClassInfoType(parents, newScope, clazz) + clazz setInfoAndEnter ClassInfoType(parents, newScope, clazz) markAllCompleted } private def newMethod(owner: Symbol, name: TermName, formals: List[Type], restpe: Type, flags: Long): MethodSymbol = { val msym = owner.newMethod(name.encode, NoPosition, flags) val params = msym.newSyntheticValueParams(formals) - msym setInfo MethodType(params, restpe) + msym setInfo MethodType(params, restpe) markAllCompleted } private def enterNewMethod(owner: Symbol, name: TermName, formals: List[Type], restpe: Type, flags: Long = 0L): MethodSymbol = owner.info.decls enter newMethod(owner, name, formals, restpe, flags) @@ -251,8 +251,8 @@ trait Definitions extends api.StandardDefinitions { } // top types - lazy val AnyClass = enterNewClass(ScalaPackageClass, tpnme.Any, Nil, ABSTRACT) - lazy val AnyRefClass = newAlias(ScalaPackageClass, tpnme.AnyRef, ObjectTpe) + lazy val AnyClass = enterNewClass(ScalaPackageClass, tpnme.Any, Nil, ABSTRACT) markAllCompleted + lazy val AnyRefClass = newAlias(ScalaPackageClass, tpnme.AnyRef, ObjectTpe) markAllCompleted lazy val ObjectClass = getRequiredClass(sn.Object.toString) // Cached types for core monomorphic classes @@ -275,7 +275,7 @@ trait Definitions extends api.StandardDefinitions { val anyval = enterNewClass(ScalaPackageClass, tpnme.AnyVal, AnyTpe :: Nil, ABSTRACT) val av_constr = anyval.newClassConstructor(NoPosition) anyval.info.decls enter av_constr - anyval + anyval markAllCompleted }).asInstanceOf[ClassSymbol] def AnyVal_getClass = getMemberMethod(AnyValClass, nme.getClass_) @@ -287,8 +287,10 @@ trait Definitions extends api.StandardDefinitions { locally { this initFlags ABSTRACT | FINAL this setInfoAndEnter ClassInfoType(List(parent.tpe), newScope, this) + this markAllCompleted } final override def isBottomClass = true + final override def isThreadsafe(purpose: SymbolOps): Boolean = true } final object NothingClass extends BottomClassSymbol(tpnme.Nothing, AnyClass) { override def isSubClass(that: Symbol) = true @@ -357,7 +359,7 @@ trait Definitions extends api.StandardDefinitions { def delayedInitMethod = getMemberMethod(DelayedInitClass, nme.delayedInit) lazy val TypeConstraintClass = requiredClass[scala.annotation.TypeConstraint] - lazy val SingletonClass = enterNewClass(ScalaPackageClass, tpnme.Singleton, AnyTpe :: Nil, ABSTRACT | TRAIT | FINAL) + lazy val SingletonClass = enterNewClass(ScalaPackageClass, tpnme.Singleton, AnyTpe :: Nil, ABSTRACT | TRAIT | FINAL) markAllCompleted lazy val SerializableClass = requiredClass[scala.Serializable] lazy val JavaSerializableClass = requiredClass[java.io.Serializable] modifyInfo fixupAsAnyTrait lazy val ComparableClass = requiredClass[java.lang.Comparable[_]] modifyInfo fixupAsAnyTrait @@ -1126,6 +1128,7 @@ trait Definitions extends api.StandardDefinitions { lazy val AnnotationDefaultAttr: ClassSymbol = { val sym = RuntimePackageClass.newClassSymbol(tpnme.AnnotationDefaultATTR, NoPosition, 0L) sym setInfo ClassInfoType(List(AnnotationClass.tpe), newScope, sym) + markAllCompleted(sym) RuntimePackageClass.info.decls.toList.filter(_.name == sym.name) match { case existing :: _ => existing.asInstanceOf[ClassSymbol] @@ -1225,7 +1228,7 @@ trait Definitions extends api.StandardDefinitions { val tparam = clazz.newSyntheticTypeParam("T0", flags) val parents = List(AnyRefTpe, parentFn(tparam)) - clazz setInfo GenPolyType(List(tparam), ClassInfoType(parents, newScope, clazz)) + clazz setInfo GenPolyType(List(tparam), ClassInfoType(parents, newScope, clazz)) markAllCompleted } def newPolyMethod(typeParamCount: Int, owner: Symbol, name: TermName, flags: Long)(createFn: PolyMethodCreator): MethodSymbol = { @@ -1236,7 +1239,7 @@ trait Definitions extends api.StandardDefinitions { case (_, restpe) => NullaryMethodType(restpe) } - msym setInfoAndEnter genPolyType(tparams, mtpe) + msym setInfoAndEnter genPolyType(tparams, mtpe) markAllCompleted } /** T1 means one type parameter. diff --git a/src/reflect/scala/reflect/internal/Importers.scala b/src/reflect/scala/reflect/internal/Importers.scala index 91ba552012..cda3e28e08 100644 --- a/src/reflect/scala/reflect/internal/Importers.scala +++ b/src/reflect/scala/reflect/internal/Importers.scala @@ -4,6 +4,7 @@ package internal import scala.collection.mutable.WeakHashMap import scala.ref.WeakReference +import scala.reflect.internal.Flags._ // SI-6241: move importers to a mirror trait Importers extends api.Importers { to: SymbolTable => @@ -87,6 +88,7 @@ trait Importers extends api.Importers { to: SymbolTable => } my setInfo GenPolyType(mytypeParams, importType(theirCore)) my setAnnotations (their.annotations map importAnnotationInfo) + markAllCompleted(my) } } } finally { @@ -142,6 +144,7 @@ trait Importers extends api.Importers { to: SymbolTable => myowner.newTypeSymbol(myname.toTypeName, mypos, myflags) } symMap.weakUpdate(their, my) + markFlagsCompleted(my)(mask = AllFlags) my setInfo recreatedSymbolCompleter(my, their) } diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala index b00380f962..3396d3f493 100644 --- a/src/reflect/scala/reflect/internal/Symbols.scala +++ b/src/reflect/scala/reflect/internal/Symbols.scala @@ -55,16 +55,6 @@ trait Symbols extends api.Symbols { self: SymbolTable => def newFreeTypeSymbol(name: TypeName, flags: Long = 0L, origin: String): FreeTypeSymbol = new FreeTypeSymbol(name, origin) initFlags flags - /** Determines whether the given information request should trigger the given symbol's completer. - * See comments to `Symbol.needsInitialize` for details. - */ - protected def shouldTriggerCompleter(symbol: Symbol, completer: Type, isFlagRelated: Boolean, mask: Long) = - completer match { - case null => false - case _: FlagAgnosticCompleter => !isFlagRelated - case _ => abort(s"unsupported completer: $completer of class ${if (completer != null) completer.getClass else null} for symbol ${symbol.fullName}") - } - /** The original owner of a class. Used by the backend to generate * EnclosingMethod attributes. */ @@ -106,12 +96,14 @@ trait Symbols extends api.Symbols { self: SymbolTable => } def knownDirectSubclasses = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize + // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method. + if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize children } def selfType = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize + // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method. + if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize typeOfThis } @@ -147,6 +139,11 @@ trait Symbols extends api.Symbols { self: SymbolTable => with Annotatable[Symbol] with Attachable { + // makes sure that all symbols that runtime reflection deals with are synchronized + private def isSynchronized = this.isInstanceOf[scala.reflect.runtime.SynchronizedSymbols#SynchronizedSymbol] + private def isAprioriThreadsafe = isThreadsafe(AllOps) + assert(isCompilerUniverse || isSynchronized || isAprioriThreadsafe, s"unsafe symbol $initName (child of $initOwner) in runtime reflection universe") + type AccessBoundaryType = Symbol type AnnotationType = AnnotationInfo @@ -616,20 +613,55 @@ trait Symbols extends api.Symbols { self: SymbolTable => && isTopLevel && nme.isReplWrapperName(name) ) + + /** In our current architecture, symbols for top-level classes and modules + * are created as dummies. Package symbols just call newClass(name) or newModule(name) and + * consider their job done. + * + * In order for such a dummy to provide meaningful info (e.g. a list of its members), + * it needs to go through unpickling. Unpickling is a process of reading Scala metadata + * from ScalaSignature annotations and assigning it to symbols and types. + * + * A single unpickling session takes a top-level class or module, parses the ScalaSignature annotation + * and then reads metadata for the unpicklee, its companion (if any) and all their members recursively + * (i.e. the pickle not only contains info about directly nested classes/modules, but also about + * classes/modules nested into those and so on). + * + * Unpickling is triggered automatically whenever typeSignature (info in compiler parlance) is called. + * This happens because package symbols assign completer thunks to the dummies they create. + * Therefore metadata loading happens lazily and transparently. + * + * Almost transparently. Unfortunately metadata isn't limited to just signatures (i.e. lists of members). + * It also includes flags (which determine e.g. whether a class is sealed or not), annotations and privateWithin. + * This gives rise to unpleasant effects like in SI-6277, when a flag test called on an uninitialize symbol + * produces incorrect results. + * + * One might think that the solution is simple: automatically call the completer + * whenever one needs flags, annotations and privateWithin - just like it's done for typeSignature. + * Unfortunately, this leads to weird crashes in scalac, and currently we can't attempt + * to fix the core of the compiler risk stability a few weeks before the final release. + * upd. Haha, "a few weeks before the final release". This surely sounds familiar :) + * + * However we do need to fix this for runtime reflection, since this idionsynchrazy is not something + * we'd like to expose to reflection users. Therefore a proposed solution is to check whether we're in a + * runtime reflection universe, and if yes and if we've not yet loaded the requested info, then to commence initialization. + */ final def getFlag(mask: Long): Long = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = true, mask = mask)) initialize + if (!isCompilerUniverse && !isThreadsafe(purpose = FlagOps(mask))) initialize flags & mask } /** Does symbol have ANY flag in `mask` set? */ final def hasFlag(mask: Long): Boolean = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = true, mask = mask)) initialize + // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method. + if (!isCompilerUniverse && !isThreadsafe(purpose = FlagOps(mask))) initialize (flags & mask) != 0 } def hasFlag(mask: Int): Boolean = hasFlag(mask.toLong) /** Does symbol have ALL the flags in `mask` set? */ final def hasAllFlags(mask: Long): Boolean = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = true, mask = mask)) initialize + // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method. + if (!isCompilerUniverse && !isThreadsafe(purpose = FlagOps(mask))) initialize (flags & mask) == mask } @@ -950,12 +982,21 @@ trait Symbols extends api.Symbols { self: SymbolTable => final def isInitialized: Boolean = validTo != NoPeriod - /** Some completers call sym.setInfo when still in-flight and then proceed with initialization (e.g. see LazyPackageType) - * setInfo sets _validTo to current period, which means that after a call to setInfo isInitialized will start returning true. - * Unfortunately, this doesn't mean that info becomes ready to be used, because subsequent initialization might change the info. - * Therefore we need this method to distinguish between initialized and really initialized symbol states. + /** We consider a symbol to be thread-safe, when multiple concurrent threads can call its methods + * (either directly or indirectly via public reflection or internal compiler infrastructure), + * without any locking and everything works as it should work. + * + * In its basic form, `isThreadsafe` always returns false. Runtime reflection augments reflection infrastructure + * with threadsafety-tracking mechanism implemented in `SynchronizedSymbol` that communicates with underlying completers + * and can sometimes return true if the symbol has been completed to the point of thread safety. + * + * The `purpose` parameter signifies whether we want to just check immutability of certain flags for the given mask. + * This is necessary to enable robust auto-initialization of `Symbol.flags` for runtime reflection, and is also quite handy + * in avoiding unnecessary initializations when requesting for flags that have already been set. */ - final def isFullyInitialized: Boolean = _validTo != NoPeriod && (flags & LOCKED) == 0 + def isThreadsafe(purpose: SymbolOps): Boolean = false + def markFlagsCompleted(mask: Long): this.type = this + def markAllCompleted(): this.type = this /** Can this symbol be loaded by a reflective mirror? * @@ -1232,7 +1273,8 @@ trait Symbols extends api.Symbols { self: SymbolTable => */ private[this] var _privateWithin: Symbol = _ def privateWithin = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize + // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method. + if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize _privateWithin } def privateWithin_=(sym: Symbol) { _privateWithin = sym } @@ -1490,46 +1532,6 @@ trait Symbols extends api.Symbols { self: SymbolTable => catch { case _: CyclicReference => debuglog("Hit cycle in maybeInitialize of $this") ; false } } - /** Called when the programmer requests information that might require initialization of the underlying symbol. - * - * `isFlagRelated` and `mask` describe the nature of this information. - * isFlagRelated = true means that the programmer needs particular bits in flags. - * isFlagRelated = false means that the request is unrelated to flags (annotations or privateWithin). - * - * In our current architecture, symbols for top-level classes and modules - * are created as dummies. Package symbols just call newClass(name) or newModule(name) and - * consider their job done. - * - * In order for such a dummy to provide meaningful info (e.g. a list of its members), - * it needs to go through unpickling. Unpickling is a process of reading Scala metadata - * from ScalaSignature annotations and assigning it to symbols and types. - * - * A single unpickling session takes a top-level class or module, parses the ScalaSignature annotation - * and then reads metadata for the unpicklee, its companion (if any) and all their members recursively - * (i.e. the pickle not only contains info about directly nested classes/modules, but also about - * classes/modules nested into those and so on). - * - * Unpickling is triggered automatically whenever typeSignature (info in compiler parlance) is called. - * This happens because package symbols assign completer thunks to the dummies they create. - * Therefore metadata loading happens lazily and transparently. - * - * Almost transparently. Unfortunately metadata isn't limited to just signatures (i.e. lists of members). - * It also includes flags (which determine e.g. whether a class is sealed or not), annotations and privateWithin. - * This gives rise to unpleasant effects like in SI-6277, when a flag test called on an uninitialize symbol - * produces incorrect results. - * - * One might think that the solution is simple: automatically call the completer whenever one needs - * flags, annotations and privateWithin - just like it's done for typeSignature. Unfortunately, this - * leads to weird crashes in scalac, and currently we can't attempt to fix the core of the compiler - * risk stability a few weeks before the final release. - * - * However we do need to fix this for runtime reflection, since it's not something we'd like to - * expose to reflection users. Therefore a proposed solution is to check whether we're in a - * runtime reflection universe and if yes then to commence initialization. - */ - protected def needsInitialize(isFlagRelated: Boolean, mask: Long) = - !isInitialized && (flags & LOCKED) == 0 && shouldTriggerCompleter(this, if (infos ne null) infos.info else null, isFlagRelated, mask) - /** Was symbol's type updated during given phase? */ final def hasTypeAt(pid: Phase#Id): Boolean = { assert(isCompilerUniverse) @@ -1688,7 +1690,8 @@ trait Symbols extends api.Symbols { self: SymbolTable => * the annotations attached to member a definition (class, method, type, field). */ def annotations: List[AnnotationInfo] = { - if (!isCompilerUniverse && needsInitialize(isFlagRelated = false, mask = 0)) initialize + // See `getFlag` to learn more about the `isThreadsafe` call in the body of this method. + if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize _annotations } @@ -3512,6 +3515,17 @@ trait Symbols extends api.Symbols { self: SymbolTable => Statistics.newView("#symbols")(ids) + +// -------------- Completion -------------------------------------------------------- + + // is used to differentiate levels of thread-safety in `Symbol.isThreadsafe` + case class SymbolOps(isFlagRelated: Boolean, mask: Long) + val AllOps = SymbolOps(isFlagRelated = false, mask = 0L) + def FlagOps(mask: Long) = SymbolOps(isFlagRelated = true, mask = mask) + + private def relevantSymbols(syms: Seq[Symbol]) = syms.flatMap(sym => List(sym, sym.moduleClass, sym.sourceModule)) + def markFlagsCompleted(syms: Symbol*)(mask: Long): Unit = relevantSymbols(syms).foreach(_.markFlagsCompleted(mask)) + def markAllCompleted(syms: Symbol*): Unit = relevantSymbols(syms).foreach(_.markAllCompleted) } object SymbolsStats { diff --git a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala index 3d222fce10..2f3e726787 100644 --- a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala +++ b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala @@ -275,6 +275,7 @@ abstract class UnPickler { def pflags = flags & PickledFlags def finishSym(sym: Symbol): Symbol = { + markFlagsCompleted(sym)(mask = AllFlags) sym.privateWithin = privateWithin sym.info = ( if (atEnd) { @@ -663,7 +664,7 @@ abstract class UnPickler { private class LazyTypeRef(i: Int) extends LazyType with FlagAgnosticCompleter { private val definedAtRunId = currentRunId private val p = phase - override def complete(sym: Symbol) : Unit = try { + protected def completeInternal(sym: Symbol) : Unit = try { val tp = at(i, () => readType(sym.isTerm)) // after NMT_TRANSITION, revert `() => readType(sym.isTerm)` to `readType` if (p ne null) slowButSafeEnteringPhase(p) (sym setInfo tp) @@ -673,6 +674,10 @@ abstract class UnPickler { catch { case e: MissingRequirementError => throw toTypeError(e) } + override def complete(sym: Symbol) : Unit = { + completeInternal(sym) + if (!isCompilerUniverse) markAllCompleted(sym) + } override def load(sym: Symbol) { complete(sym) } } @@ -680,8 +685,9 @@ abstract class UnPickler { * of completed symbol to symbol at index `j`. */ private class LazyTypeRefAndAlias(i: Int, j: Int) extends LazyTypeRef(i) { - override def complete(sym: Symbol) = try { - super.complete(sym) + override def completeInternal(sym: Symbol) = try { + super.completeInternal(sym) + var alias = at(j, readSymbol) if (alias.isOverloaded) alias = slowButSafeEnteringPhase(picklerPhase)((alias suchThat (alt => sym.tpe =:= sym.owner.thisType.memberType(alt)))) diff --git a/src/reflect/scala/reflect/runtime/JavaMirrors.scala b/src/reflect/scala/reflect/runtime/JavaMirrors.scala index c8bdece272..68c67bb1f8 100644 --- a/src/reflect/scala/reflect/runtime/JavaMirrors.scala +++ b/src/reflect/scala/reflect/runtime/JavaMirrors.scala @@ -63,10 +63,10 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni private[reflect] lazy val runDefinitions = new definitions.RunDefinitions // only one "run" in the reflection universe import runDefinitions._ - override lazy val RootPackage = new RootPackage with SynchronizedTermSymbol - override lazy val RootClass = new RootClass with SynchronizedModuleClassSymbol - override lazy val EmptyPackage = new EmptyPackage with SynchronizedTermSymbol - override lazy val EmptyPackageClass = new EmptyPackageClass with SynchronizedModuleClassSymbol + override lazy val RootPackage = (new RootPackage with SynchronizedTermSymbol).markFlagsCompleted(mask = AllFlags) + override lazy val RootClass = (new RootClass with SynchronizedModuleClassSymbol).markFlagsCompleted(mask = AllFlags) + override lazy val EmptyPackage = (new EmptyPackage with SynchronizedTermSymbol).markFlagsCompleted(mask = AllFlags) + override lazy val EmptyPackageClass = (new EmptyPackageClass with SynchronizedModuleClassSymbol).markFlagsCompleted(mask = AllFlags) /** The lazy type for root. */ @@ -575,6 +575,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni val bytes = ssig.getBytes val len = ByteCodecs.decode(bytes) unpickler.unpickle(bytes take len, 0, clazz, module, jclazz.getName) + markAllCompleted(clazz, module) case None => loadBytes[Array[String]]("scala.reflect.ScalaLongSignature") match { case Some(slsig) => @@ -583,6 +584,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni val len = ByteCodecs.decode(encoded) val decoded = encoded.take(len) unpickler.unpickle(decoded, 0, clazz, module, jclazz.getName) + markAllCompleted(clazz, module) case None => // class does not have a Scala signature; it's a Java class info("translating reflection info for Java " + jclazz) //debug @@ -605,6 +607,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni private def createTypeParameter(jtvar: jTypeVariable[_ <: GenericDeclaration]): TypeSymbol = { val tparam = sOwner(jtvar).newTypeParameter(newTypeName(jtvar.getName)) .setInfo(new TypeParamCompleter(jtvar)) + markFlagsCompleted(tparam)(mask = AllFlags) tparamCache enter (jtvar, tparam) tparam } @@ -617,6 +620,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni override def load(sym: Symbol) = complete(sym) override def complete(sym: Symbol) = { sym setInfo TypeBounds.upper(glb(jtvar.getBounds.toList map typeToScala map objToAny)) + markAllCompleted(sym) } } @@ -664,6 +668,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni module setFlag (flags & PRIVATE | JAVA) module.moduleClass setFlag (flags & PRIVATE | JAVA) } + markFlagsCompleted(clazz, module)(mask = AllFlags) /** used to avoid cycles while initializing classes */ private var parentsLevel = 0 @@ -688,6 +693,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni override def complete(sym: Symbol): Unit = { load(sym) completeRest() + markAllCompleted(clazz, module) } def completeRest(): Unit = gilSynchronized { @@ -740,6 +746,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni class LazyPolyType(override val typeParams: List[Symbol]) extends LazyType with FlagAgnosticCompleter { override def complete(sym: Symbol) { completeRest() + markAllCompleted(clazz, module) } } } @@ -894,6 +901,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni val pkg = owner.newPackage(name) pkg.moduleClass setInfo new LazyPackageType pkg setInfoAndEnter pkg.moduleClass.tpe + markFlagsCompleted(pkg)(mask = AllFlags) info("made Scala "+pkg) pkg } else @@ -1076,6 +1084,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni fieldCache.enter(jfield, field) propagatePackageBoundary(jfield, field) copyAnnotations(field, jfield) + markAllCompleted(field) field } @@ -1102,11 +1111,9 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni setMethType(meth, tparams, paramtpes, resulttpe) propagatePackageBoundary(jmeth.javaFlags, meth) copyAnnotations(meth, jmeth) - - if (jmeth.javaFlags.isVarargs) - meth modifyInfo arrayToRepeated - else - meth + if (jmeth.javaFlags.isVarargs) meth modifyInfo arrayToRepeated + markAllCompleted(meth) + meth } /** @@ -1129,6 +1136,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni constr setInfo GenPolyType(tparams, MethodType(clazz.newSyntheticValueParams(paramtpes), clazz.tpe)) propagatePackageBoundary(jconstr.javaFlags, constr) copyAnnotations(constr, jconstr) + markAllCompleted(constr) constr } diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala index 3a41edd28b..907314b9bd 100644 --- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala +++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala @@ -200,6 +200,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.NoSymbol this.CyclicReference // inaccessible: this.TypeHistory + this.SymbolOps this.TermName this.TypeName this.Liftable diff --git a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala index 30a3855d70..c56bc28d90 100644 --- a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala +++ b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala @@ -6,6 +6,7 @@ import internal.Flags import java.lang.{Class => jClass, Package => jPackage} import scala.collection.mutable import scala.reflect.runtime.ReflectionUtils.scalacShouldntLoadClass +import scala.reflect.internal.Flags._ private[reflect] trait SymbolLoaders { self: SymbolTable => @@ -17,6 +18,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable => * is found, a package is created instead. */ class TopClassCompleter(clazz: Symbol, module: Symbol) extends SymLoader with FlagAssigningCompleter { + markFlagsCompleted(clazz, module)(mask = ~TopLevelPickledFlags) override def complete(sym: Symbol) = { debugInfo("completing "+sym+"/"+clazz.fullName) assert(sym == clazz || sym == module || sym == module.moduleClass) @@ -24,6 +26,8 @@ private[reflect] trait SymbolLoaders { self: SymbolTable => val loadingMirror = mirrorThatLoaded(sym) val javaClass = loadingMirror.javaClass(clazz.javaClassName) loadingMirror.unpickleClass(clazz, module, javaClass) + // NOTE: can't mark as thread-safe here, because unpickleClass might decide to delegate to FromJavaClassCompleter + // if (!isCompilerUniverse) markAllCompleted(clazz, module) } } override def load(sym: Symbol) = complete(sym) @@ -64,6 +68,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable => sym setInfo new ClassInfoType(List(), new PackageScope(sym), sym) // override def safeToString = pkgClass.toString openPackageModule(sym) + markAllCompleted(sym) } } diff --git a/src/reflect/scala/reflect/runtime/SymbolTable.scala b/src/reflect/scala/reflect/runtime/SymbolTable.scala index ddbf3bd629..02155578f8 100644 --- a/src/reflect/scala/reflect/runtime/SymbolTable.scala +++ b/src/reflect/scala/reflect/runtime/SymbolTable.scala @@ -28,19 +28,4 @@ private[scala] trait SymbolTable extends internal.SymbolTable with JavaMirrors w * in order to prevent memory leaks: http://groups.google.com/group/scala-internals/browse_thread/thread/eabcf3d406dab8b2. */ override def isCompilerUniverse = false - - /** Unlike compiler universes, reflective universes can auto-initialize symbols on flag requests. - * - * scalac wasn't designed with such auto-initialization in mind, and quite often it makes assumptions - * that flag requests won't cause initialization. Therefore enabling auto-init leads to cyclic errors. - * We could probably fix those, but at the moment it's too risky. - * - * Reflective universes share codebase with scalac, but their surface is much smaller, which means less assumptions. - * These assumptions are taken care of in this overriden `shouldTriggerCompleter` method. - */ - override protected def shouldTriggerCompleter(symbol: Symbol, completer: Type, isFlagRelated: Boolean, mask: Long) = - completer match { - case _: TopClassCompleter | _: JavaClassCompleter => !isFlagRelated || (mask & TopLevelPickledFlags) != 0 - case _ => super.shouldTriggerCompleter(symbol, completer, isFlagRelated, mask) - } } diff --git a/src/reflect/scala/reflect/runtime/SynchronizedSymbols.scala b/src/reflect/scala/reflect/runtime/SynchronizedSymbols.scala index d7e3d3c84b..fe1de77cd2 100644 --- a/src/reflect/scala/reflect/runtime/SynchronizedSymbols.scala +++ b/src/reflect/scala/reflect/runtime/SynchronizedSymbols.scala @@ -4,6 +4,7 @@ package runtime import scala.reflect.io.AbstractFile import scala.collection.{ immutable, mutable } +import scala.reflect.internal.Flags._ private[reflect] trait SynchronizedSymbols extends internal.Symbols { self: SymbolTable => @@ -31,23 +32,103 @@ private[reflect] trait SynchronizedSymbols extends internal.Symbols { self: Symb trait SynchronizedSymbol extends Symbol { - def gilSynchronizedIfNotInited[T](body: => T): T = { - // TODO JZ desired, but prone to race conditions. We need the runtime reflection based - // type completers to establish a memory barrier upon initialization. Maybe a volatile - // write? We need to consult with the experts here. Until them, lock pessimistically. - // - // `run/reflection-sync-subtypes.scala` fails about 1/50 times otherwise. - // - // if (isFullyInitialized) body - // else gilSynchronized { body } - gilSynchronized { body } + /** (Things written in this comment only applies to runtime reflection. Compile-time reflection, + * especially across phases and runs, is somewhat more complicated, but we won't be touching it, + * because at the moment we only care about synchronizing runtime reflection). + * + * As it has been noted on multiple occasions, generally speaking, reflection artifacts aren't thread-safe. + * Reasons for that differ from artifact to artifact. In some cases it's quite bad (e.g. types use a number + * of non-concurrent compiler caches, so we need to serialize certain operations on types in order to make + * sure that things stay deterministic). However, in case of symbols there's hope, because it's only during + * initializaton that symbols are thread-unsafe. After everything's set up, symbols become immutable + * (sans a few deterministic caches that can be populated simultaneously by multiple threads) and therefore thread-safe. + * + * Note that by saying "symbols become immutable" I mean literally that. In a very common case of PackageClassSymbol's, + * even when a symbol finishes its initialization and becomes immutable, its info forever remains mutable. + * Therefore even if we no longer need to synchronize a PackageClassSymbol after it's initialized, we still have to take + * care of its ClassInfoType (or, more precisely, of the underlying Scope), but that's done elsewhere, and + * here we don't need to worry about that. + * + * Okay, so now we simply check `Symbol.isInitialized` and if it's true, then everything's fine? Haha, nope! + * The thing is that some completers call sym.setInfo when still in-flight and then proceed with initialization + * (e.g. see LazyPackageType). Consequently, setInfo sets _validTo to current period, which means that after + * a call to setInfo isInitialized will start returning true. Unfortunately, this doesn't mean that info becomes + * ready to be used, because subsequent initialization might change the info. + * + * Therefore we need to somehow distinguish between initialized and really initialized symbol states. + * Okay, let's do it on per-completer basis. We have seven kinds of completers to worry about: + * 1) LazyPackageType that initializes packages and their underlying package classes + * 2) TopClassCompleter that initializes top-level Scala-based class-module companion pairs of static definitions + * 3) LazyTypeRef and LazyTypeRefAndAlias set up by TopClassCompleter that initialize (transitive members) of top-level classes/modules + * 4) FromJavaClassCompleter that does the same for both top-level and non-toplevel Java-based classes/modules + * 5) Fully-initialized signatures of non-class/module Java-based reflection artifacts + * 6) Importing completer that transfers metadata from one universe to another + * 7) Signatures of special symbols such as roots and symbolsNotPresentInBytecode + * + * The mechanisms underlying completion are quite complex, and it'd be only natural to suppose that over time we're going to overlook something. + * Wrt isThreadsafe we could have two wrong situations: false positives (isThreadsafe = true, but the symbol isn't actually threadsafe) + * and false negatives (isThreadsafe = false, but the symbol is actually threadsafe). However, even though both are wrong, only the former + * is actively malicious. Indeed, false positives might lead to races, inconsistent state and crashes, while the latter would only cause + * `initialize` to be called and a gil to be taken on every potentially auto-initializable operation. Unpleasant yes, but still robust. + * + * What makes me hopeful is that: + * 1) By default (e.g. if some new completion mechanism gets introduced for a special flavor of symbols and we forget to call markCompleted) + * isThreadsafe is always in false negative state, which is unpleasant but safe. + * 2) Calls to `markCompleted` which are the only potential source of erroneous behavior are few and are relatively easy to place: + * just put them just before your completer's `complete` returns, and you should be fine. + * + * upd. Actually, there's another problem of not keeping initialization mask up-to-date. If we're not careful enough, + * then it might so happen that getting a certain flag that the compiler assumes to be definitively set will spuriously + * return isThreadsafe(purpose = FlagsOp(<flag>)) = false and that will lead to spurious auto-initialization, + * which will cause an SO or a cyclic reference or some other crash. I've done my best to go through all possible completers + * and call `markFlagsCompleted` where appropriate, but again over time something might be overlooked, so to guard against that + * I'm only considering TopLevelPickledFlags to be sources of potential initialization. This ensures that such system flags as + * isMethod, isModule or isPackage are never going to auto-initialize. + */ + override def isThreadsafe(purpose: SymbolOps) = { + if (isCompilerUniverse) false + else if (_initialized) true + else purpose.isFlagRelated && (_initializationMask & purpose.mask & TopLevelPickledFlags) == 0 } - override def validTo = gilSynchronizedIfNotInited { super.validTo } - override def info = gilSynchronizedIfNotInited { super.info } - override def rawInfo: Type = gilSynchronizedIfNotInited { super.rawInfo } + /** Communicates with completers declared in scala.reflect.runtime.SymbolLoaders + * about the status of initialization of the underlying symbol. + * + * Unfortunately, it's not as easy as just introducing the `markThreadsafe` method that would be called + * by the completers when they are really done (as opposed to `setInfo` that, as mentioned above, doesn't mean anything). + * + * Since we also want to auto-initialize symbols when certain methods are being called (`Symbol.hasFlag` for example), + * we need to track the identity of the initializer, so as to block until initialization is complete if the caller + * comes from a different thread, but to skip auto-initialization if we're the initializing thread. + * + * Just a volatile var is fine, because: + * 1) Status can only be changed in a single-threaded fashion (this is enforced by gilSynchronized + * that effecively guards `Symbol.initialize`), which means that there can't be update conflicts. + * 2) If someone reads a stale value of status, then the worst thing that might happen is that this someone + * is going to spuriously call `initialize`, which is either a gil-protected operation (if the symbol isn't inited yet) + * or a no-op (if the symbol is already inited), and that is fine in both cases. + * + * upd. It looks like we also need to keep track of a mask of initialized flags to make sure + * that normal symbol initialization routines don't trigger auto-init in Symbol.flags-related routines (e.g. Symbol.getFlag). + * Due to the same reasoning as above, a single volatile var is enough for to store the mask. + */ + @volatile private[this] var _initialized = false + @volatile private[this] var _initializationMask = TopLevelPickledFlags + override def markFlagsCompleted(mask: Long): this.type = { _initializationMask = _initializationMask & ~mask; this } + override def markAllCompleted(): this.type = { _initializationMask = 0L; _initialized = true; this } + + def gilSynchronizedIfNotThreadsafe[T](body: => T): T = { + if (isCompilerUniverse || isThreadsafe(purpose = AllOps)) body + else gilSynchronized { body } + } + + override def validTo = gilSynchronizedIfNotThreadsafe { super.validTo } + override def info = gilSynchronizedIfNotThreadsafe { super.info } + override def rawInfo: Type = gilSynchronizedIfNotThreadsafe { super.rawInfo } + override def typeSignature: Type = gilSynchronizedIfNotThreadsafe { super.typeSignature } + override def typeSignatureIn(site: Type): Type = gilSynchronizedIfNotThreadsafe { super.typeSignatureIn(site) } - override def typeParams: List[Symbol] = gilSynchronizedIfNotInited { + override def typeParams: List[Symbol] = gilSynchronizedIfNotThreadsafe { if (isCompilerUniverse) super.typeParams else { if (isMonomorphicType) Nil @@ -63,7 +144,7 @@ private[reflect] trait SynchronizedSymbols extends internal.Symbols { self: Symb } } } - override def unsafeTypeParams: List[Symbol] = gilSynchronizedIfNotInited { + override def unsafeTypeParams: List[Symbol] = gilSynchronizedIfNotThreadsafe { if (isCompilerUniverse) super.unsafeTypeParams else { if (isMonomorphicType) Nil @@ -124,7 +205,7 @@ private[reflect] trait SynchronizedSymbols extends internal.Symbols { self: Symb // we can keep this lock fine-grained, because it's just a cache over asSeenFrom, which makes deadlocks impossible // unfortunately we cannot elide this lock, because the cache depends on `pre` private lazy val typeAsMemberOfLock = new Object - override def typeAsMemberOf(pre: Type): Type = gilSynchronizedIfNotInited { typeAsMemberOfLock.synchronized { super.typeAsMemberOf(pre) } } + override def typeAsMemberOf(pre: Type): Type = gilSynchronizedIfNotThreadsafe { typeAsMemberOfLock.synchronized { super.typeAsMemberOf(pre) } } } trait SynchronizedModuleSymbol extends ModuleSymbol with SynchronizedTermSymbol @@ -133,7 +214,7 @@ private[reflect] trait SynchronizedSymbols extends internal.Symbols { self: Symb // unlike with typeConstructor, a lock is necessary here, because tpe calculation relies on // temporarily assigning NoType to tpeCache to detect cyclic reference errors private lazy val tpeLock = new Object - override def tpe_* : Type = gilSynchronizedIfNotInited { tpeLock.synchronized { super.tpe_* } } + override def tpe_* : Type = gilSynchronizedIfNotThreadsafe { tpeLock.synchronized { super.tpe_* } } } trait SynchronizedClassSymbol extends ClassSymbol with SynchronizedTypeSymbol diff --git a/test/pending/run/reflection-sync-potpourri.scala b/test/files/run/reflection-sync-potpourri.scala index 0c96974df7..0c96974df7 100644 --- a/test/pending/run/reflection-sync-potpourri.scala +++ b/test/files/run/reflection-sync-potpourri.scala |