Closed scabug closed 9 years ago
Imported From: https://issues.scala-lang.org/browse/SI-9375?orig=1 Reporter: @lrytz Affected Versions: 2.11.7
@lrytz said (edited on Jul 28, 2015 7:43:12 PM UTC):
I think the correct fix is to change the readResolve
method of the nested object to the following:
def readResolve() = {
if ($outer.attr$module == null) this
else $outer.attr$module
}
Since the field (attr$module
) is private, we might create another name-mangled accessor for it (instead of making the field public)
Q: should the method be synchronized?
CC @axel22
@lrytz said: CC @DarkDimius
@ijuma said: Is this a new issue in 2.11.7 or something that has been there for a while?
@lrytz said: It's an old bug. I just tested with 2.10.3, same there.
@ijuma said: OK, thanks. :)
@axel22 said:
Lukas, what you propose fixes this problem, but we have to additionally make sure that all the objects that see Outer
, agree on the same instance of the nested object attr
.
If we are deserializing only an Outer
instance along with its attr
object, then this will indeed be the case.
However, there might be multiple instances of the attr
object created per an Outer
class instance if some other code invokes the attr()
method between the time when the proposed attr.readResolve
returns this
and the time when Outer
class is fully deserialized. The only situation where this could happen (that I can think of) is if the nested object has a custom readObject
method:
class Outer extends Serializable {
def foo = attr
object attr extends Serializable {
println("Ctor!")
var x = "default"
override def toString = s"attr($x)"
private def readObject(in: ObjectInputStream) {
println("dro begin")
in.defaultReadObject
println("dro done")
foo
}
}
override def toString = "Outer - " + attr
}
// Exiting paste mode, now interpreting.
defined class Outer
scala> val outer = new Outer
Ctor!
outer: Outer = Outer - attr(default)
scala> outer.attr.x = "custom"
outer.attr.x: String = custom
scala> serializeDeserialize(outer)
dro begin
dro done
Ctor!
res11: Outer = Outer - attr(default)
To avoid this situation, the readResolve
method should set the attr$module
field the first time that the singleton instance is used:
def readResolve() = {
if ($outer.attr$module == null) {
$outer.attr$module = this
this
} else $outer.attr$module
}
I would avoid synchronization here, since it could result in subtle deadlocks, but does not buy you much.
In fact, if the field checked by the attr()
accessor is really not a volatile, then the initialization of the nested object is not thread-safe already.
@lrytz said (edited on Jul 20, 2015 9:37:06 AM UTC): Thanks for you comment, Alex.
I'm not sure your suggestion would help in the situation you describe though. It seems readResolve
is called only after readObject
returns (example below). So we'd get this sequence of events:
Outer
instanceattr$
instance for the attr$module
fieldattr$.readObject
outer$.attr
getter, which finds the attr$module
field still being null
, so a new module instance is created (constructor side-effects!)attr$.readResolve
outer$.attr$module
field is non-null, so its value is returnedI don't see a way to prevent invoking the module constructor if the module is accessed by a custom readObject
.
case class C(var s: String) {
private def readObject(in: ObjectInputStream): Unit = {
println("Before Default")
println(this)
in.defaultReadObject
println("After Default")
println(this)
this.s = "readObject"
println(this)
}
private def readResolve(): Object = {
println("readResolve")
C("readResolve")
}
}
scala> val c = C("orig")
c: C = C(orig)
scala> serializeDeserialize(c)
Before Default
C(null)
After Default
C(orig)
C(readObject)
readResolve
res2: C = C(readResolve)
Adam Pocock (craigacp) said (edited on Jul 21, 2015 1:03:26 PM UTC): Our workaround for this bug is to call defaultReadObject in attr, then call Outer.attr, which does freshly instantiate the module and we then copy the values over. We wouldn't need this when the inner class de-serialises correctly, but it would have very counterintuitive behaviour if we needed to do computation at de-serialization time (for example reconstructing computed fields) and accidentally accessed attr instead of the this pointer.
@lrytz said: very early WIP: https://github.com/scala/scala/compare/2.12.x...lrytz:t9375?expand=1
@axel22 said: Quick answer to whether it should be synchronized: no, as far as I can see.
Long answer: I didn't check what gets generated, but if the following accessor is generated:
// generated accesssor:
// public attr()LOuter$attr$ = {
// if (attr$module == null) attr$lzycompute()
// else attr$module
// }
and at the same time, the attr$module
field is not volatile, then the lazy accessors for nested objects are not thread-safe -- it is possible that some thread reads a non-null
attr$module
reference to a nested object whose constructor did not yet complete (because the statements inside the synchronized
block can be reordered with the constructor body of singleton object).
This bug could manifest itself:
a) during concurrent access to Outer
's attr
field
b) possibly during serialization if the reference to the Outer
object somehow escapes to another thread
So, to fix this bug, the accessor method does not need to be synchronized.
However, the long-term fix should be to make attr$module
volatile:
synchronized
in attr$lzycompute
fix the problem (a)Outer
object reference escaping to another thread, this should also fix (b), as far as I can see@lrytz said: after some thought and i-don't-tell-how-many-hours of working on the prototype for my initial idea, i'm quite convinced now that the right solution is simply to NOT emit a readResolve method. Submitted in https://github.com/scala/scala/pull/4671.
@axel22 said: Wow, nice. That sounds about right. It should not be necessary to ensure singletoness on the nested object, since both the nested object and its surrounding object are fresh and unique after deserialization anyway.
Still, I believe that my comment about the attr$module
being volatile stands - that would make nested singleton objects more consistent with the top-level singletons.
Adam Pocock (craigacp) said: Is it possible to get the fix backported into a 2.11 release?
@lrytz said: Backport PR: https://github.com/scala/scala/pull/4757. I went for 2.12.x because my original attempt would not have been binary compatible, but the way it was finally solved is.