Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization in 2.13.0-M5 #254

Closed
ashawley opened this issue Aug 30, 2018 · 9 comments
Closed

Serialization in 2.13.0-M5 #254

ashawley opened this issue Aug 30, 2018 · 9 comments

Comments

@ashawley
Copy link
Member

I tried compiling on the latest M5 with Seth's branch and get serialization issues:

[error] Test scala.xml.SerializationTest.unmatched failed: expected:<> but was:<List()>, took 0.078 sec
[error] Test scala.xml.SerializationTest.xmlLiteral failed: expected:<<node/>> but was:<List(scala.collection.generic.DefaultSerializationProxy@28ec9fc9)>, took 0.013 sec
[error] Test scala.xml.SerializationTest.implicitConversion failed: expected:<<child></child><child/>> but was:<List(List(scala.collection.generic.DefaultSerializationProxy@5844dad8), List(scala.collection.generic.DefaultSerializationProxy@3204a005))>, took 0.002 sec
[error] Test scala.xml.SerializationTest.empty failed: expected:<> but was:<List()>, took 0.001 sec
[error] Test scala.xml.XMLTestJVM.serializeAttribute failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Attribute, took 0.011 sec
[error]     at scala.xml.XMLTestJVM.serializeAttribute(XMLTest.scala:218)
[error]     ...
[error] Test scala.xml.XMLTestJVM.serializeDocument failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Document, took 0.016 sec
[error]     at scala.xml.XMLTestJVM.serializeDocument(XMLTest.scala:228)
[error]     ...
[error] Test scala.xml.XMLTestJVM.serializeElem failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Elem, took 0.002 sec
[error]     at scala.xml.XMLTestJVM.serializeElem(XMLTest.scala:236)
[error]     ...
[error] Test scala.xml.XMLTestJVM.serializeComplex failed: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to scala.xml.Elem, took 0.082 sec
[error]     at scala.xml.XMLTestJVM.serializeComplex(XMLTest.scala:271)
[error]     ...
[error] Failed: Total 164, Failed 8, Errors 0, Passed 156
[error] Failed tests:
[error] 	scala.xml.XMLTestJVM
[error] 	scala.xml.SerializationTest

When I look in the last passing 2.13 community build from Aug-22 I only see scala-xml getting skipped:

[scala-xml] --== Building scala-xml ==--
[scala-xml] Found cached project build, uuid 4cab6219ea2340373d5bbf922cd70ba828d91bfd
[scala-xml] --== End Building scala-xml ==--

According to @xuwei-k in comment in scala-xml#253, the issue is related to changes

Should we change scala.Iterable#writeReplace implementation from the viewpoint of compatibility and liskov substitution principle? 🤔
scala/scala#6676

I'm not sure how @xuwei-k found this so quickly, but indeed xuwei-k/scala-xml@97bc6b4 fixed my issue.

commit 97bc6b4e9e09b91937528ddf5a2d131cbac278ec (xuwei-k/writeReplace)
Author: xuwei-k
Date:   Thu Aug 30 09:09:04 2018 +0900

    override writeReplace. fix serialize error

diff --git a/shared/src/main/scala/scala/xml/MetaData.scala b/shared/src/main/scala/scala/xml/MetaData.scala
index 1affff4e..cdb98851 100644
--- a/shared/src/main/scala/scala/xml/MetaData.scala
+++ b/shared/src/main/scala/scala/xml/MetaData.scala
@@ -225,4 +225,6 @@ abstract class MetaData
 
   final def remove(namespace: String, owner: Node, key: String): MetaData =
     remove(namespace, owner.scope, key)
+
+  protected[this] override def writeReplace(): AnyRef = this
 }
diff --git a/shared/src/main/scala/scala/xml/NodeSeq.scala b/shared/src/main/scala/scala/xml/NodeSeq.scala
index 97b2ddf2..c99fe978 100644
--- a/shared/src/main/scala/scala/xml/NodeSeq.scala
+++ b/shared/src/main/scala/scala/xml/NodeSeq.scala
@@ -155,4 +155,6 @@ abstract class NodeSeq extends AbstractSeq[Node] with immutable.Seq[Node] with S
   override def toString(): String = theSeq.mkString
 
   def text: String = (this map (_.text)).mkString
+
+  protected[this] override def writeReplace(): AnyRef = this
 }
@ashawley
Copy link
Member Author

ashawley commented Aug 30, 2018

I don't know enough about the broad consequences to promote this to a Scala bug with a minimal example, although it seems it should.

As is, we would need to have a mix-in trait that would provide writeReplace to fix this in 2.13, and then a no-op version for 2.12 and earlier.

I'm surprised this wasn't caught earlier in the community build. If the issue is scala/scala#6676 as @xuwei-k states, then it would have been an issue since it was merged on May-25. If true, then scala-xml in the community build was producing a false positive and giving a false sense of security?

I'm not surprised that it didn't get caught in the compiler test suite since scala-xml and a few tests were removed early in M5 scala/scala#6436 on Apr-25. Some partests for XML literals were added back in scala/scala#6569 on Apr-27, but they wouldn't have caught this serialization issue.

@xuwei-k
Copy link
Contributor

xuwei-k commented Aug 30, 2018

excluded some tests in community-builds 😢
scala/community-build@87e67ea#diff-13924878884a607db34ef8aa8c07a253R200

@xuwei-k
Copy link
Contributor

xuwei-k commented Aug 30, 2018

minimal example

package example

import java.io._

class MyCollection[B](val list: List[B]) extends scala.collection.Iterable[B] {
  override def iterator = list.iterator

  // protected[this] override def writeReplace(): AnyRef = this
}

object Main {
  def write[A](o: A): Array[Byte] = {
    val ba = new ByteArrayOutputStream(512)
    val out = new ObjectOutputStream(ba)
    out.writeObject(o)
    out.close()
    ba.toByteArray()
  }

  def read[A](buffer: Array[Byte]): A = {
    val in = new ObjectInputStream(new ByteArrayInputStream(buffer))
    in.readObject().asInstanceOf[A]
  }

  def main(args: Array[String]): Unit = {
    val list = List(1, 2, 3)

    // success
    assert(read[List[Int]](write(list)) == list)

    // fail
    assert(read[MyCollection[Int]](write(new MyCollection(list))).list == list)
  }
}

@lrytz lrytz assigned szeiger and unassigned szeiger Aug 30, 2018
@lrytz
Copy link
Member

lrytz commented Aug 30, 2018

cc @szeiger

@SethTisue
Copy link
Member

umbrella ticket for serialization issue(s) in 2.13: scala/scala-dev#562

@ashawley
Copy link
Member Author

bug report officially filed scala/bug#11192

@ashawley
Copy link
Member Author

I just closed the 2.13 WIP pull request that included the strange workaround for this Scala 2.13 bug, but it would be great to take it out in the future.

@SethTisue
Copy link
Member

scala/scala#7624 cements the new serialization scheme, so this isn't going to be "fixed" any further on the Scala end.

the typical workarounds are to either:

  • set fork in Test := true
  • use a custom classloader that works around the JVM flaw which is the root cause

sbt 1.3 will use a flatter classloader structure that we believe will make the fork around unnecessary

@ashawley
Copy link
Member Author

The specific serialization issue was fixed, so we can close. I should have done that after merging #276.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants