Skip to content
Permalink
Browse files
Fix deadlock issue with custom Enum implementation
Our Enum implementation use for property values/lookups has important
benefits over Scala Enumerations. However, it seems like the way we
implement it can lead to circular deadlocks. When a Value is initialized
it causes the Enum to be initialized. And the Enum initialized calls
forceConstruction to initialized all the Values. This leads to circular
instantiating that works in most cases, but sometimes leads to a
deadlock. Where this deadlock exists or how to fix it is not clear. It's
also not clear what changed to make this deadlock much more likely.

To avoid this, we this change removes the forceConstruction function and
replaces it with a manually managed Array of Enum Values. This Array is
lazily evaluated so that instantiated an Enum does not also directly
instantiate the Enum Value, avoiding the circular deadlock. This does
require extra code to define an Enum, but the majority of Enums are in
generated code so doesn't add much maintenance burden.

DAFFODIL-2704
  • Loading branch information
stevedlawrence committed Jun 16, 2022
1 parent 6c9cc40 commit 0f5ebcab73bc54394e58597b6e44d36ddfbfd8ee
Show file tree
Hide file tree
Showing 10 changed files with 55 additions and 58 deletions.
@@ -204,7 +204,7 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments) {
val optImplementation = TDMLImplementation.optionStringToEnum("implementation", s)
if (!optImplementation.isDefined) {
throw new Exception("Unrecognized TDML implementation '%s'. Must be one of %s"
.format(s, TDMLImplementation.allValues.mkString(", ")))
.format(s, TDMLImplementation.values.mkString(", ")))
}
optImplementation.get
})
@@ -409,7 +409,7 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments) {

val implementation = opt[TDMLImplementation](short = 'I', argName = "implementation",
descr = "Implementation to run TDML tests. Choose one of %s. Defaults to %s."
.format(TDMLImplementation.allValues.mkString(", "), TDMLImplementation.Daffodil.toString),
.format(TDMLImplementation.values.mkString(", "), TDMLImplementation.Daffodil.toString),
default = None)
val info = tally(descr = "Increment test result information output level, one level for each -i")
val list = opt[Boolean](descr = "Show names and descriptions instead of running test cases")
@@ -30,9 +30,9 @@ object MyProp extends Enum[MyPropType] // with ThrowsSDE
lazy val context = Fakes.fakeElem
// lazy val schemaComponent = context
case object PropVal1 extends MyPropType
forceConstruction(PropVal1)
case object PropVal2 extends MyPropType
forceConstruction(PropVal2)
override lazy val values = Array(PropVal1, PropVal2)

def apply(name: String): MyPropType = apply(name, context)
def apply(name: String, context: ThrowsSDE) = stringToEnum("myProp", name, context)
}
@@ -48,7 +48,7 @@ class TestPropertyRuntime {
@Test
def testConstructed(): Unit = {
// val myPropUser = new RealObject
val av = MyProp.allValues
val av = MyProp.values
val pv1 = MyProp.PropVal1
val pv2 = MyProp.PropVal2
assertTrue(av.contains(pv1))
@@ -85,9 +85,10 @@ class HasMixin extends SchemaComponentImpl(<foo/>, None)

sealed trait TheExampleProp extends TheExampleProp.Value
object TheExampleProp extends Enum[TheExampleProp] {
case object Left extends TheExampleProp; forceConstruction(Left)
case object Right extends TheExampleProp; forceConstruction(Right)
case object Center extends TheExampleProp; forceConstruction(Center)
case object Left extends TheExampleProp
case object Right extends TheExampleProp
case object Center extends TheExampleProp
override lazy val values = Array(Left, Right, Center)

def apply(name: String, self: ThrowsSDE): TheExampleProp = stringToEnum("theExampleProp", name, self)
}
@@ -58,6 +58,8 @@ import passera.unsigned.ULong
sealed trait AlignmentType extends AlignmentType.Value
object AlignmentType extends Enum[AnyRef] { // Note: Was using AlignmentUnits mixin here!
case object Implicit extends AlignmentType
override lazy val values = Array(Implicit)

val allowedAlignmentValues = {
val ints = 0 to 30 // that's every perfect power of 2 that fits in an Int.
ints.map(1 << _)
@@ -126,10 +128,11 @@ trait TextStandardBaseMixin extends PropertyMixin {

sealed trait SeparatorSuppressionPolicy extends SeparatorSuppressionPolicy.Value
object SeparatorSuppressionPolicy extends Enum[SeparatorSuppressionPolicy] {
case object Never extends SeparatorSuppressionPolicy; forceConstruction(Never)
case object TrailingEmpty extends SeparatorSuppressionPolicy; forceConstruction(TrailingEmpty)
case object TrailingEmptyStrict extends SeparatorSuppressionPolicy; forceConstruction(TrailingEmptyStrict)
case object AnyEmpty extends SeparatorSuppressionPolicy; forceConstruction(AnyEmpty)
case object Never extends SeparatorSuppressionPolicy
case object TrailingEmpty extends SeparatorSuppressionPolicy
case object TrailingEmptyStrict extends SeparatorSuppressionPolicy
case object AnyEmpty extends SeparatorSuppressionPolicy
override lazy val values = Array(Never, TrailingEmpty, TrailingEmptyStrict, AnyEmpty)

def apply(name: String, self: ThrowsSDE): SeparatorSuppressionPolicy = stringToEnum("separatorSuppressionPolicy", name, self)
}
@@ -427,8 +430,9 @@ trait TextStandardExponentRepMixin extends PropertyMixin {
*/
sealed trait EmptyElementParsePolicy extends EmptyElementParsePolicy.Value
object EmptyElementParsePolicy extends Enum[EmptyElementParsePolicy] {
case object TreatAsMissing extends EmptyElementParsePolicy; forceConstruction(TreatAsMissing)
case object TreatAsEmpty extends EmptyElementParsePolicy; forceConstruction(TreatAsEmpty)
case object TreatAsMissing extends EmptyElementParsePolicy
case object TreatAsEmpty extends EmptyElementParsePolicy
override lazy val values = Array(TreatAsMissing, TreatAsEmpty)

def apply(name: String, context: ThrowsSDE): EmptyElementParsePolicy = stringToEnum("emptyElementParsePolicy", name, context)
}
@@ -21,7 +21,6 @@ import org.apache.daffodil.exceptions._
import org.apache.daffodil.util.Misc._
import org.apache.daffodil.util._
import org.apache.daffodil.cookers.Converter
import scala.collection.mutable

/**
* Enum class as basis for our DFDL properties
@@ -46,9 +45,10 @@ import scala.collection.mutable
*
* sealed trait BinaryNumberRep extends BinaryNumberRep.Value
* object BinaryNumberRep extends Enum[BinaryNumberRep] {
* case object Packed extends BinaryNumberRep ; forceConstruction(Packed)
* case object Bcd extends BinaryNumberRep ; forceConstruction(Bcd)
* case object Binary extends BinaryNumberRep ; forceConstruction(Binary)
* case object Packed extends BinaryNumberRep
* case object Bcd extends BinaryNumberRep
* case object Binary extends BinaryNumberRep
* override lazy val values = Array(Packed, Bcd, Binary)
*
* def apply(name: String) : BinaryNumberRep = stringToEnum("binaryNumberRep", name)
* }
@@ -84,7 +84,8 @@ import scala.collection.mutable
abstract class EnumBase
abstract class EnumValueBase extends Serializable
abstract class Enum[A] extends EnumBase with Converter[String, A] {
class Value extends EnumValueBase { self: A => {
class Value extends EnumValueBase { self: A =>
override lazy val toString = {
val theVal = this
val cn = getNameFromClass(this)
val en = cn match {
@@ -94,18 +95,13 @@ abstract class Enum[A] extends EnumBase with Converter[String, A] {
case "Sunday" | "Monday" | "Tuesday" | "Wednesday" | "Thursday" | "Friday" | "Saturday" => cn
case _ => Misc.toInitialLowerCaseUnlessAllUpperCase(cn)
}
_values += (en -> theVal)
_values
}
override lazy val toString = {
val propName = Misc.toInitialLowerCaseUnlessAllUpperCase(getNameFromClass(this))
propName
en
}
}

def toPropName(prop: A) = prop.toString

private val _values = mutable.ArrayBuffer.empty[Tuple2[String, A]]
val values: Array[A]

/**
* This is invoked at runtime to compare expression results to see if they
@@ -121,29 +117,15 @@ abstract class Enum[A] extends EnumBase with Converter[String, A] {

def optionStringToEnum(enumTypeName: String, str: String): Option[A] = {
var i: Int = 0
while (i < _values.size) {
if (_values(i)._1.equals(str)) { // was equals ignore case - that's just going to allow errors if used at runtime
return Some(_values(i)._2)
while (i < values.size) {
if (values(i).toString.equals(str)) {
return Some(values(i))
}
i += 1
}
None
}

/**
* Useful for diagnostic messages where you want to say "must be one of ...." and list the possibilities.
*/
def allValues = _values.map { case (k, v) => v }

/**
* Scala delays construction of case objects (presumably because many programs don't use them at all)
* We need to force creation of our inner property case objects because constructing them also has
* the side-effect of registering them in the _values list.
*/
def forceConstruction(obj: Any): Unit = {
//Assert.invariant(obj.toString() != "") // TODO: is forceConstruction needed at all?
}

override protected def convert(b: String, context: ThrowsSDE, forUnparse: Boolean): A = apply(b, context)

def apply(name: String, context: ThrowsSDE): A
@@ -167,7 +167,7 @@ class TestGeneratedProperties {
comparePropValue(hasProps.calendarCheckPolicy, "lax")
comparePropValue(hasProps.calendarTimeZone, "UTC")
comparePropValue(hasProps.calendarObserveDST, "yes")
comparePropValue(hasProps.calendarFirstDayOfWeek, "monday")
comparePropValue(hasProps.calendarFirstDayOfWeek, "Monday")
comparePropValue(hasProps.calendarDaysInFirstWeek, "4")
comparePropValue(hasProps.calendarCenturyStart, "53")
comparePropValue(hasProps.occursCountKind, "parsed")
@@ -340,7 +340,7 @@ class PropertyGenerator(arg: Node) {
object Currency extends Enum[Currency] {
"""
val templateMiddle =
""" case object EUR extends Currency ; forceConstruction(EUR)
""" case object EUR extends Currency
"""

// Modified to pass the context, so diagnostics can be better.
@@ -425,19 +425,17 @@ trait CurrencyMixin extends PropertyMixin {
val traitName = initialUpperCase(pname)
val propName = initialLowerCase(pname)
val middle = templateMiddle.replaceAll("Currency", traitName)
val mids = pvalues.map(pvalue => {
// need to insure the enum values aren't digits (as in the TextNumberBase enum)
val pvalueAsIdentifier = if (pvalue.charAt(0).isDigit) "INTEGER_" + pvalue else pvalue
middle.replace("EUR", initialUpperCase(pvalueAsIdentifier))
})
val pvalueIDs = pvalues.map(pvalue => initialUpperCase(pvalue))
val mids = pvalueIDs.map(id => middle.replace("EUR", id))
val values = pvalueIDs.mkString(" override lazy val values = Array(", ", ", ")\n")
val start = templateStart.replaceAll("Currency", traitName)
val end = templateEnd.replaceAll("Currency", traitName).replaceAll("currency", propName)
val mixin =
if (excludeRuntimeProperties(propName)) "\n"
else {
templateMixin.replaceAll("Currency", traitName).replaceAll("currency", propName)
}
val res = start + mids.foldLeft("")(_ + _) + end + mixin
val res = start + mids.foldLeft("")(_ + _) + values + end + mixin
res
}

@@ -351,7 +351,12 @@ class TunableEnumDefinition(schemaRootConfig: scala.xml.Node, schemaRootExt: sca

private val scalaEnums = {
val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
scalaEnumValues.map { e => s""" case object ${e} extends ${scalaType}; forceConstruction(${e})""" }
scalaEnumValues.map { e => s""" case object ${e} extends ${scalaType}""" }
}

private val values = {
val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
scalaEnumValues.mkString(" override lazy val values = Array(", ", ", ")")
}

private val bottom = s"""
@@ -360,7 +365,7 @@ class TunableEnumDefinition(schemaRootConfig: scala.xml.Node, schemaRootExt: sca
""".stripMargin

val scalaEnumeration = {
top + "\n" + scalaEnums.mkString("\n") + "\n" + bottom
top + "\n" + scalaEnums.mkString("\n") + "\n" + values + "\n" + bottom
}

}
@@ -71,12 +71,18 @@ class WarnIDGenerator(schema: scala.xml.Node) {
w.write(top)
w.write("\n")

enumerationNodes.foreach { node =>
val scalaNames = enumerationNodes.map { node =>
val enumName = node \@ "value"
val scalaName = enumName.head.toUpper + enumName.tail
w.write(s" case object ${scalaName} extends WarnID; forceConstruction($scalaName)\n")
scalaName
}

scalaNames.foreach { scalaName =>
w.write(s" case object ${scalaName} extends WarnID\n")
}

w.write(scalaNames.mkString(" override lazy val values = Array(", ", ", ")\n"))

w.write("\n")
w.write(bottom)
w.flush();
@@ -126,8 +126,9 @@ sealed trait LineFoldMode extends LineFoldMode.Value {

object LineFoldMode extends Enum[LineFoldMode] {

case object IMF extends LineFoldMode; forceConstruction(Left)
case object iCalendar extends LineFoldMode; forceConstruction(Right)
case object IMF extends LineFoldMode
case object iCalendar extends LineFoldMode
override lazy val values = Array(IMF, iCalendar)

override def apply(name: String, context: ThrowsSDE): LineFoldMode = stringToEnum("lineFoldMode", name, context)
}
@@ -98,7 +98,7 @@ object Runner {
* A test or test suite can override this to specify more or different implementations
* that the test should pass for.
*/
def defaultImplementationsDefaultDefault = TDMLImplementation.allValues.map(_.toString)
def defaultImplementationsDefaultDefault = TDMLImplementation.values.map(_.toString)

/**
* By default we don't run Daffodil negative TDML tests against cross-testers.

0 comments on commit 0f5ebca

Please sign in to comment.