[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #197: Fix non-thread safe code related to text date times

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #197: Fix non-thread safe code related to text date times
URL: https://github.com/apache/incubator-daffodil/pull/197#discussion_r266057369
 
 

 ##########
 File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ConvertTextCalendarUnparser.scala
 ##########
 @@ -42,51 +42,72 @@ case class ConvertTextCalendarUnparser(
   override lazy val runtimeDependencies = Vector(localeEv, calendarEv)
 
   def unparse(state: UState): Unit = {
-    val locale: ULocale = localeEv.evaluate(state)
-    val calendar: Calendar = calendarEv.evaluate(state)
 
     val node = state.currentInfosetNode.asSimple
 
     val dc = node.dataValue
 
-    val calValue = node.dataValue match {
+    val infosetCalendar = node.dataValue match {
       case dc: DFDLCalendar => dc.calendar
       case x => Assert.invariantFailed("ConvertTextCalendar received unsupported type. %s of type %s.".format(x, Misc.getNameFromClass(x)))
     }
 
-    // This initialization is needed because the calendar object may have been
-    // persisted, and that computes/completes fields that are not yet
-    // completed, such as the Julian day, which freezes the year to 1970. We
-    // want a fresh start on all the fields that are filled in from a parse.
-    // Also, the tlDataFormatter below uses a cache based on the value of the
-    // calendar, so we want to ensure we always get the same data formatter if
-    // the calendar is the same
-    calendar.clear()
+    val locale: ULocale = localeEv.evaluate(state)
+
+    val calendarOrig: Calendar = calendarEv.evaluate(state)
 
-    val df = tlDataFormatter(locale, calendar)
-    df.setCalendar(calendar)
+    // The clear() here actually shouldn't be necessary since we call clear()
+    // when we create the calendar in CalendarEv, and nothing ever modifies
+    // that calendar--we only modify clones. However, it looks like the act of
+    // deserializing a Calendar object causes values to be initialized again.
+    // So if someone uses a saved parser this calendar will have garbage in it
+    // that can affect the results. So clear it here to make sure that's not
+    // the case.
+    calendarOrig.clear()
 
-    // At this point we should be able to just do "df.format(calValue)".
-    // However, when we do that ICU actually uses some fields in calValue (e.g.
+    // It's important here to use the calendarOrig that results from
+    // calendarEv.evaluate() since the tlDataFormatter is a cache the uses
+    // reference equality. For everything else we want to use a clone for
+    // reasons described below.
+    val df = tlDataFormatter(locale, calendarOrig)
+
+    // When we evaluate calendarEV, if it is a constant we will always get back
+    // the same Calendar object. Because of this it is important here to clone
+    // this calendar and always use the clone below for two reasons:
+    //
+    // 1) The below code will modify the calendar object based on the dateTime
+    //    in the infoset. Any changes to the object would persist and affect
+    //    future unparses. By cloning, we ensure we do not modify the original
+    //    calendar object that other unparses will use.
+    //
+    // 2) Multiple threads would have access to the same Calendar object, and
+    //    so the below code could modify the same object at the same time
+    //    across threads. By cloning, we ensure that they modify different
+    //    objects.
+    val calendar = calendarOrig.clone().asInstanceOf[Calendar]
 
 Review comment:
   Yeah, it's pretty unfortunate. We could alternatively do thread local stuff, but it just becomes a pain and is more complex. And I'm not sure if there's a big gain, I would imagine the synchronizing that a thread local requires is more overhead that allocating a new Calendar object--it's not a super complex object.
   
   Ultimately, we should switch to library that isn't so stateful. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services