Merge branch 'release/0.3.4'

johnynek · johnynek · commit 1a007bdd5bdc · 2013-10-04T18:00:20.000-07:00
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,5 +1,8 @@
 # chill #
 
+### 0.3.4
+* Bugfixes for Externalizer with looped object graphs https://github.com/twitter/chill/pull/143
+
 ### 0.3.3
 * Serialize synthetic fields by default: https://github.com/twitter/chill/pull/135
 * Prefer Java to Kryo, but check both in Externalizer: https://github.com/twitter/chill/pull/138
diff --git a/README.md b/README.md
@@ -147,7 +147,7 @@ Discussion occurs primarily on the [Chill mailing list](https://groups.google.co
 
 ## Maven
 
-Chill modules are available on Maven Central. The current groupid and version for all modules is, respectively, `"com.twitter"` and  `0.3.2`.
+Chill modules are available on Maven Central. The current groupid and version for all modules is, respectively, `"com.twitter"` and  `0.3.3`.
 
 Current published artifacts are
 
diff --git a/chill-scala/src/main/scala/com/twitter/chill/Externalizer.scala b/chill-scala/src/main/scala/com/twitter/chill/Externalizer.scala
@@ -25,7 +25,16 @@ import _root_.java.io.{
   ObjectOutputStream
 }
 
+import com.esotericsoftware.kryo.serializers.JavaSerializer
+import com.esotericsoftware.kryo.DefaultSerializer
+import _root_.java.util.concurrent.atomic.{AtomicBoolean, AtomicReference}
+import com.esotericsoftware.kryo.KryoSerializable
+
 object Externalizer {
+  /* Tokens used to distinguish if we used Kryo or Java */
+  private val KRYO = 0
+  private val JAVA = 1
+
   def apply[T](t: T): Externalizer[T] = {
     val x = new Externalizer[T]
     x.set(t)
@@ -39,24 +48,38 @@ object Externalizer {
  * work. Of course, Java serialization may fail if the contained
  * item is not Java serializable
  */
-class Externalizer[T] extends Externalizable {
-  private var item: Option[T] = None
+class Externalizer[T] extends Externalizable with KryoSerializable {
+  // Either points to a result or a delegate Externalizer to fufil that result.
+  private var item: Either[Externalizer[T], Option[T]] = Right(None)
+  import Externalizer._
+
+  @transient private val doesJavaWork = new AtomicReference[Option[Boolean]](None)
+  @transient private val testing = new AtomicBoolean(false)
+  // For backwards compatibility
+  private def KRYO = Externalizer.KRYO
 
-  def getOption: Option[T] = item
-  def get: T = item.get // This should never be None when get is called
+  // No vals or var's below this line!
+
+  def getOption: Option[T] = item match {
+    case Left(e) => e.getOption
+    case Right(i) => i
+  }
+
+  def get: T = getOption.get // This should never be None when get is called
 
   /** Unfortunately, Java serialization requires mutable objects if
    * you are going to control how the serialization is done.
    * Use the companion object to creat new instances of this
    */
   def set(it: T): Unit = {
-    assert(item.isEmpty, "Tried to call .set on an already constructed Externalizer")
-    item = Some(it)
+    item match {
+      case Left(e) => e.set(it)
+      case Right(x) =>
+        assert(x.isEmpty, "Tried to call .set on an already constructed Externalizer")
+        item = Right(Some(it))
+    }
   }
 
-  /* Tokens used to distinguish if we used Kryo or Java */
-  private val KRYO = 0
-  private val JAVA = 1
 
   /** Override this to configure Kryo creation with a named subclass,
    * e.g.
@@ -70,35 +93,47 @@ class Externalizer[T] extends Externalizable {
     (new ScalaKryoInstantiator).setReferences(true)
 
   // 1 here is 1 thread, since we will likely only serialize once
-  private val kpool = KryoPool.withByteArrayOutputStream(1, kryo)
+  // this should not be a val because we don't want to capture a reference
+
+
+  def javaWorks: Boolean =
+    doesJavaWork.get match {
+      case Some(v) => v
+      case None => probeJavaWorks
+    }
 
   /** Try to round-trip and see if it works without error
    */
-  lazy val javaWorks: Boolean = {
+  private def probeJavaWorks: Boolean = {
+    if(!testing.compareAndSet(false, true)) return true
     try {
       val baos = new ByteArrayOutputStream()
       val oos = new ObjectOutputStream(baos)
-      oos.writeObject(item)
+      oos.writeObject(getOption)
       val bytes = baos.toByteArray
       val testInput = new ByteArrayInputStream(bytes)
       val ois = new ObjectInputStream(testInput)
       ois.readObject // this may throw
+      doesJavaWork.set(Some(true))
       true
     }
     catch {
       case t: Throwable =>
         Option(System.getenv.get("CHILL_EXTERNALIZER_DEBUG"))
           .filter(_.toBoolean)
           .foreach { _ => t.printStackTrace }
+        doesJavaWork.set(Some(false))
         false
     }
+    finally {
+      testing.set(false)
+    }
   }
 
-  private def safeToBytes: Option[Array[Byte]] = {
+  private def safeToBytes(kryo: KryoInstantiator): Option[Array[Byte]] = {
     try {
-      val bytes = kpool.toBytesWithClass(item)
-      // Make sure we can read without throwing
-      fromBytes(bytes)
+      val kpool = KryoPool.withByteArrayOutputStream(1, kryo)
+      val bytes = kpool.toBytesWithClass(getOption)
       Some(bytes)
     }
     catch {
@@ -109,41 +144,80 @@ class Externalizer[T] extends Externalizable {
         None
     }
   }
-  private def fromBytes(b: Array[Byte]): Option[T] =
-    kpool.fromBytes(b).asInstanceOf[Option[T]]
+  private def fromBytes(b: Array[Byte], kryo: KryoInstantiator): Option[T] =
+    KryoPool.withByteArrayOutputStream(1, kryo)
+      .fromBytes(b)
+      .asInstanceOf[Option[T]]
 
-  def readExternal(in: ObjectInput) {
+  override def readExternal(in: ObjectInput) = maybeReadJavaKryo(in, kryo)
+
+  private def maybeReadJavaKryo(in: ObjectInput, kryo: KryoInstantiator) {
     in.read match {
       case JAVA =>
-        item = in.readObject.asInstanceOf[Option[T]]
+        item = Right(in.readObject.asInstanceOf[Option[T]])
       case KRYO =>
         val sz = in.readInt
         val buf = new Array[Byte](sz)
         in.readFully(buf)
-        item = fromBytes(buf)
+        item = Right(fromBytes(buf, kryo))
     }
   }
 
   protected def writeJava(out: ObjectOutput): Boolean =
     javaWorks && {
       out.write(JAVA)
-      out.writeObject(item)
+      out.writeObject(getOption)
       true
     }
 
-  protected def writeKryo(out: ObjectOutput): Boolean =
-    safeToBytes.map { bytes =>
+  protected def writeKryo(out: ObjectOutput): Boolean = writeKryo(out, kryo)
+
+  protected def writeKryo(out: ObjectOutput, kryo: KryoInstantiator): Boolean =
+    safeToBytes(kryo).map { bytes =>
       out.write(KRYO)
       out.writeInt(bytes.size)
       out.write(bytes)
       true
     }.getOrElse(false)
 
-  def writeExternal(out: ObjectOutput) {
-    writeJava(out) || writeKryo(out) || {
-      val inner = item.get
+  private def maybeWriteJavaKryo(out: ObjectOutput, kryo: KryoInstantiator) {
+    writeJava(out) || writeKryo(out, kryo) || {
+      val inner = get
       sys.error("Neither Java nor Kyro works for class: %s instance: %s\nexport CHILL_EXTERNALIZER_DEBUG=true to see both stack traces"
         .format(inner.getClass, inner))
     }
   }
+
+  override def writeExternal(out: ObjectOutput) = maybeWriteJavaKryo(out, kryo)
+
+  def write (kryo: Kryo, output: Output): Unit = {
+    val resolver = kryo.getReferenceResolver
+    resolver.getWrittenId(item) match {
+      case -1 =>
+        output.writeInt(-1)
+        resolver.addWrittenObject(item)
+        val oStream = new ObjectOutputStream(output)
+        maybeWriteJavaKryo(oStream, () => kryo)
+        oStream.flush
+      case n =>
+        output.writeInt(n)
+    }
+  }
+
+  def read (kryo: Kryo, input: Input): Unit = {
+    doesJavaWork.set(None)
+    testing.set(false)
+    val state = input.readInt()
+    val resolver = kryo.getReferenceResolver
+    state match {
+      case -1 =>
+      val objId = resolver.nextReadId(this.getClass)
+        resolver.setReadObject(objId, this)
+        maybeReadJavaKryo(new ObjectInputStream(input), () => kryo)
+      case n =>
+        val z = resolver.getReadObject(this.getClass, n).asInstanceOf[Externalizer[T]]
+        if(!(z eq this)) item = Left(z)
+    }
+  }
+
 }
diff --git a/chill-scala/src/test/scala/com/twitter/chill/ExternalizerSpec.scala b/chill-scala/src/test/scala/com/twitter/chill/ExternalizerSpec.scala
@@ -0,0 +1,98 @@
+/*
+Copyright 2012 Twitter, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package com.twitter.chill
+
+import org.specs._
+
+import scala.collection.immutable.BitSet
+import scala.collection.immutable.ListMap
+import scala.collection.immutable.HashMap
+
+import scala.collection.mutable.{ArrayBuffer => MArrayBuffer, HashMap => MHashMap}
+import _root_.java.util.PriorityQueue
+import _root_.java.util.Locale
+import scala.collection.mutable
+
+class ExtSomeRandom(val x: Int)
+
+class ExternalizerSpec extends Specification with BaseProperties {
+
+  noDetailedDiffs() //Fixes issue for scala 2.9
+
+  def getKryo = KryoSerializer.registered.newKryo
+
+  "KryoSerializers and KryoDeserializers" should {
+    "Externalizer handle circular references with Java" in {
+      val l = Array[AnyRef]("asdf", "defs")
+      val ext = Externalizer(l)
+      l(1) = ext
+
+      ext.javaWorks must be_==(true)
+    }
+    "Externalizer handle circular references with Java2" in {
+      val l = Array[AnyRef](null)
+      val ext = Externalizer(l)
+      l.update(0, ext) // make a loop
+      (l(0) eq ext) must beTrue
+      ext.javaWorks must be_==(true)
+      //jrt(ext).get.toList must_==(l.toList)
+      // Try Kryo also
+      //rt(ext).get.toList must_==(l.toList)
+ 
+      val nonJavaSer = Array(new ExtSomeRandom(2))
+      jrt(nonJavaSer) must throwA[Exception]
+      val ext2 = Externalizer(nonJavaSer)
+      jrt(ext2).get(0).x must be_==(2)
+      ext2.javaWorks must beFalse
+ 
+     
+    }
+
+    "Externalizer handle circular references with kryo only serialzable objects" in {
+       // Add on non-java serialziable and a loop
+      val l3 = Array[AnyRef](null, null)
+      val ext3 = Externalizer(l3)
+      l3.update(0, ext3) // make a loop
+      l3.update(1, new ExtSomeRandom(3)) // make a loop
+      (l3(0) eq ext3) must beTrue
+      ext3.javaWorks must be_==(false)
+      (jrt(ext3).get)(1).asInstanceOf[ExtSomeRandom].x must_==(l3(1).asInstanceOf[ExtSomeRandom].x)
+    }
+
+    "Externalizer circular reference with scala tuples(java and kryo Serializable" in {
+      val l4 = Array[AnyRef](null, null)
+      val ext4 = Externalizer(l4)
+      l4.update(0, ext4) // make a loop
+      l4.update(1, (3, 7)) // make a loop
+      (l4(0) eq ext4) must beTrue
+      ext4.javaWorks must be_==(true)
+      (rt(ext4).get)(1) must_==(l4(1))
+      (jrt(ext4).get)(1) must_==(l4(1))
+    }
+
+    "Externalizer handle circular references with non Kryo Serializable members" in {
+        val l4 = Array[AnyRef](null, null)
+        val ext4 = Externalizer(l4)
+        l4.update(0, ext4) // make a loop
+        l4.update(1, new Locale("en")) // make a loop
+        (l4(0) eq ext4) must beTrue
+        ext4.javaWorks must be_==(true)
+        (rt(new EmptyScalaKryoInstantiator(), ext4).get)(1)
+        (jrt(ext4).get)(1) must_==(l4(1))
+    }
+  }
+}
diff --git a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala
@@ -24,6 +24,7 @@ import scala.collection.immutable.HashMap
 
 import scala.collection.mutable.{ArrayBuffer => MArrayBuffer, HashMap => MHashMap}
 import _root_.java.util.PriorityQueue
+import _root_.java.util.Locale
 import scala.collection.mutable
 /*
 * This is just a test case for Kryo to deal with. It should
@@ -163,6 +164,11 @@ class KryoSpec extends Specification with BaseProperties {
       ext.javaWorks must be_==(false)
       jrt(ext).get.x must_==(l.x)
     }
+    "Externalizer can RT with Kryo" in {
+      val l = new SomeRandom(10)
+      val ext = Externalizer(l)
+      rt(ext).get.x must_==(l.x)
+    }
     "handle Regex" in {
       val test = """\bhilarious""".r
       val roundtripped = rt(test)
diff --git a/project/Build.scala b/project/Build.scala
@@ -12,7 +12,7 @@ object ChillBuild extends Build {
 
   val sharedSettings = Project.defaultSettings ++ mimaDefaultSettings ++ Seq(
 
-    version := "0.3.3",
+    version := "0.3.4",
     organization := "com.twitter",
     scalaVersion := "2.9.3",
     crossScalaVersions := Seq("2.9.3", "2.10.0"),
@@ -122,7 +122,7 @@ object ChillBuild extends Build {
     settings = sharedSettings
   ).settings(
     name := "chill",
-    previousArtifact := Some("com.twitter" % "chill_2.9.3" % "0.3.0"),
+    previousArtifact := Some("com.twitter" % "chill_2.9.3" % "0.3.3"),
     libraryDependencies ++= Seq(
       "org.ow2.asm" % "asm-commons" % "4.0"
     )
@@ -156,7 +156,7 @@ object ChillBuild extends Build {
       "Clojars Repository" at "http://clojars.org/repo",
       "Conjars Repository" at "http://conjars.org/repo"
     ),
-    libraryDependencies += "storm" % "storm" % "0.9.0-wip9"
+    libraryDependencies += "storm" % "storm" % "0.9.0-wip9" % "provided"
   ).dependsOn(chillJava)
 
   // This can only have java deps!
