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

SI-8549 Serialization: fix regression with @SerialVersionUID / start enforcing backwards compatibility #3711

Merged
merged 3 commits into from May 8, 2014

Conversation

retronym
Copy link
Member

@retronym retronym commented May 5, 2014

No description provided.

@retronym
Copy link
Member Author

retronym commented May 5, 2014

Review by @lrytz @gkossakowski

@retronym
Copy link
Member Author

retronym commented May 5, 2014

I'm also considering doing a sweep though the collections to add missing @SerialVersionUID annotations. Of course, we first need to agree on a good criteria for "missing".

One option here is to add this to AbstractTraversable, as the annotation can be defined up the class hierarchy.

@retronym
Copy link
Member Author

retronym commented May 5, 2014

The meta discussion we can have here is how to integrate this into a Scala 2.11.1.

  • Wait for our regular release date
  • Release now
    • From master
    • From v2.11.0 with just this change cherry-picked

@retronym retronym changed the title SI-8459 Serialization: fix regression with @SerialVersionUID / start enforing backwards compatibility SI-8549 Serialization: fix regression with @SerialVersionUID / start enforing backwards compatibility May 5, 2014
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was
changed from a `StaticAnnotation` to `ClassFileAnnotation` in
order to avoid silently ignoring non-literal UIDs like:

    @serialversionuid(0 - 12345L) class C

And to flag non-constant UIDs:

    @serialversionuid("!!!".length)

While this indeed was fold constants, the change was incomplete.
The compiler API for reading the argument from a `ClassFileAnnoation`
is different, on must look for a `LiteralAnnotArg`, rather than a
`Literal`.

This commit:

  - amends the backend accordingly
  - removes relevant duplication between `GenASM` and `GenBCode`
  - tests that the static field is generated accordingly

This will mean that we will break deserialization of objects from
Scalal 2.11.0 that use this annotation.
@retronym retronym changed the title SI-8549 Serialization: fix regression with @SerialVersionUID / start enforing backwards compatibility SI-8549 Serialization: fix regression with @SerialVersionUID / start enforcing backwards compatibility May 5, 2014
@@ -42,6 +42,9 @@ import scala.collection.mutable.{ ArrayBuilder, WrappedArray }
@scala.annotation.implicitNotFound(msg = "No Manifest available for ${T}.")
// TODO undeprecated until Scala reflection becomes non-experimental
// @deprecated("Use scala.reflect.ClassTag (to capture erasures) or scala.reflect.runtime.universe.TypeTag (to capture types) or both instead", "2.10.0")
// TODO 2.12 Use named classes, marked with `SerialVersionUID`, rather than anonymous classes for predefined class tags
// This will provide for better for better serialization stability. Update run/t8549.scala to check that values
// of types that we declare as Serializable are not instances of anoynmous classes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we log such TODOs in the issue tracker?f

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've opened https://issues.scala-lang.org/browse/SI-8561 and removed these comments.

@lrytz
Copy link
Member

lrytz commented May 5, 2014

LGTM

To date, we've been hesidant to offer any guarantees about
Java serialization of standard library types among heteregenous
Scala versions.

Nonetheless, we have added `SerialVersionUID` annotations to
parts of the standard library, to offer some stability. This
protects against two winds of change: automatic calculation of
this UID might differ between JVM versions, or it might differ
due to otherwise immaterial changes to the library in Scala
releases.

With this commit, we strengthen the guarantees. Classes
marked with `SerialVersionUID` will be serialization compatible
within minor releases of Scala. This is backed up by the
enclosed test.

After major releases, we reserve the right to break this.
But the test will serve to avoid *accidental* changes.

Specifically, the test case checks:

  - deserialize(serialize(x)) == x
  - serialize(x) is stable over time

I have included values of all types marked with `@SerialVersionUID`
in the library. For some types, I've added variations in the
values to exercise different subclasses, such as `Set1` / `Set2`.

This found that that the serialized form of predefined `ClassTags`
included the cached identity hash code and failed the stability
test. This wasn't an issue for correctness as they also provide
`readResolve`, but I marked those fields as `@transient` in any
case to comply with the test expectations.

That whole area is good example of a serialization worst-practice:
using anonymous classes in code like:

    val Object: Manifest[java.lang.Object] = new PhantomManifest[...](...) {
       private def readResolve(): Any = Manifest.AnyVal
    }

... will lead to instability if these declarations are shifted around
in the file. Named classes would be preferred. I've noted this in a
TODO comment for 2.12.
The `neg` test was already working since `SerialVersionUID`
was changed to a `ClassFileAnnotation`; the `run` test only
started working since the recently preceding commit that
made a compensatory test in the backend.
@retronym
Copy link
Member Author

retronym commented May 5, 2014

I forgot to locally re-locker before setting the baseline for the serialization tests. That's why the tests failed: my library was still compiled with 2.11.0, which ignored @SerialVersionUID.

I just pushed a fix for the expectations.

@gkossakowski
Copy link
Member

@retronym: I'd vote vote for 2.11.1 release done ASAP and dedicated specifically to this issue (cherry picked commit).

@lrytz
Copy link
Member

lrytz commented May 5, 2014

I'd vote vote for 2.11.1 release done ASAP and dedicated specifically to this issue

+1

@retronym retronym mentioned this pull request May 7, 2014
@gkossakowski
Copy link
Member

@retronym: what's the decision on this PR?

@retronym
Copy link
Member Author

retronym commented May 7, 2014

We'll have another chat about it in the scala meeting tomorrow.

retronym added a commit that referenced this pull request May 8, 2014
SI-8549 Serialization: fix regression with @serialversionuid / start enforcing backwards compatibility
@retronym retronym merged commit a4e56ef into scala:master May 8, 2014
@adriaanm
Copy link
Contributor

adriaanm commented May 9, 2014

One concrete example of why I'm against cherry-picking: other post 2.11.0 commits are pretty relevant for 2.11.1, such as the updates to versions.properties (a9e211a) and build.number (8301b8a). More generally, we should deviate from our usual flow as little as possible: simply moving up the release date has least impact. Cherry-picking may reduce the risk of regression by only including commit, but the omission of commits can also cause regressions...

@gkossakowski
Copy link
Member

I'm ok not cherry-picking in this case.

However, I think we should work towards enabling us to cut releases around a single bug fix. We might encounter a much more serious problem in the future and we should have a process of handling such scenario. I think we are not that far from getting there.

I'll be looking into concerns like that when working on infrastructure this year.

@adriaanm
Copy link
Contributor

adriaanm commented May 9, 2014

To me, the point is that we should not merge to a release branch unless it
can go into the next release cut from that branch.
Thus, for a hot fix, submit a PR, merge it, and tag the RC, let the
automation do its thing.

On Fri, May 9, 2014 at 4:27 PM, Grzegorz Kossakowski <
notifications@github.com> wrote:

I'm ok not cherry-picking in this case.

However, I think we should work towards enabling us to cut releases around
a single bug fix. We might encounter a much more serious problem in the
future and we should have a process of handling such scenario. I think we
are not that far from getting there.

I'll be looking into concerns like that when working on infrastructure
this year.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3711#issuecomment-42671811
.

JOIN US. REGISTER TODAY! http://www.scaladays.org/
Scala http://www.scaladays.org/
Days http://www.scaladays.org/
June 16th-18th, http://www.scaladays.org/
Berlin http://www.scaladays.org/

@gkossakowski
Copy link
Member

By pushing the same line of logic a little bit further, we shouldn't need RCs for minor releases. Given that we have dbuild coverage now it might be possible. I doubt we got more testing from people manually trying out RCs for minor versions.

@adriaanm
Copy link
Contributor

adriaanm commented May 9, 2014

Interesting. Let's discuss during the next meeting. We could indeed go much
faster with minor versions, simply releasing another minor instead of an RC
if a serious enough regression is found.

On Fri, May 9, 2014 at 4:34 PM, Grzegorz Kossakowski <
notifications@github.com> wrote:

By pushing the same line of logic a little bit further, we shouldn't need
RCs for minor releases. Given that we have dbuild coverage now it might be
possible. I doubt we got more testing from people manually trying out RCs
for minor versions.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3711#issuecomment-42672642
.

JOIN US. REGISTER TODAY! http://www.scaladays.org/
Scala http://www.scaladays.org/
Days http://www.scaladays.org/
June 16th-18th, http://www.scaladays.org/
Berlin http://www.scaladays.org/

@gkossakowski
Copy link
Member

I'll remember to bring this up.

On 9 May 2014 16:37, Adriaan Moors notifications@github.com wrote:

Interesting. Let's discuss during the next meeting. We could indeed go
much
faster with minor versions, simply releasing another minor instead of an
RC
if a serious enough regression is found.

On Fri, May 9, 2014 at 4:34 PM, Grzegorz Kossakowski <
notifications@github.com> wrote:

By pushing the same line of logic a little bit further, we shouldn't
need
RCs for minor releases. Given that we have dbuild coverage now it might
be
possible. I doubt we got more testing from people manually trying out
RCs
for minor versions.


Reply to this email directly or view it on GitHub<
https://github.com/scala/scala/pull/3711#issuecomment-42672642>
.

JOIN US. REGISTER TODAY! http://www.scaladays.org/
Scala http://www.scaladays.org/
Days http://www.scaladays.org/
June 16th-18th, http://www.scaladays.org/
Berlin http://www.scaladays.org/


Reply to this email directly or view it on GitHubhttps://github.com//pull/3711#issuecomment-42672979
.

Grzegorz Kossakowski
Scalac hacker at Typesafe http://www.typesafe.com/
twitter: @gkossakowski http://twitter.com/gkossakowski
github: @gkossakowski http://github.com/gkossakowski

lrytz added a commit to lrytz/scala that referenced this pull request Nov 5, 2014
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was
changed from a `StaticAnnotation` to `ClassFileAnnotation` in
order to enforce annotation arguments to be constants.

The ID value in the AnnotationInfo moved from `args` to `assocs`, but
the backend was not adjusted. This was fixed in PR scala#3711 / ecbc9d0.

Unfortunately, the synthetic AnnotationInfo that is added to anonymous
function classes still used the old constructor (`args` instead of
`assocs`), so extracting the value failed, and no field was added to
the classfile.
lrytz added a commit to lrytz/scala that referenced this pull request Nov 5, 2014
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was changed
from a `StaticAnnotation` to `ClassFileAnnotation` in order to enforce
annotation arguments to be constants. That was 2.11.0.

The ID value in the AnnotationInfo moved from `args` to `assocs`, but
the backend was not adjusted. This was fixed in PR scala#3711 / ecbc9d0 for
2.11.1.

Unfortunately, the synthetic AnnotationInfo that is added to anonymous
function classes still used the old constructor (`args` instead of
`assocs`), so extracting the value failed, and no field was added to
the classfile.
lrytz added a commit to lrytz/scala that referenced this pull request Nov 5, 2014
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was changed
from a `StaticAnnotation` to `ClassFileAnnotation` in order to enforce
annotation arguments to be constants. That was 2.11.0.

The ID value in the AnnotationInfo moved from `args` to `assocs`, but
the backend was not adjusted. This was fixed in PR scala#3711 / ecbc9d0 for
2.11.1.

Unfortunately, the synthetic AnnotationInfo that is added to anonymous
function classes still used the old constructor (`args` instead of
`assocs`), so extracting the value failed, and no field was added to
the classfile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants