Skip to content

SI-9197 Duration.Inf not a singleton when deserialized #4416

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

Merged
merged 1 commit into from
Apr 23, 2015

Conversation

Ichoran
Copy link
Contributor

@Ichoran Ichoran commented Mar 31, 2015

Made Duration.Undefined, .Inf, and .MinusInf all give back the singleton instance instead of creating a new copy by overriding readResolve.

This override can be (and is) private, which at least on Sun's JDK8 doesn't mess with the auto-generated SerialVersionUIDs.

Thus, the patch should make things strictly better: if you're on 2.11.7+ on JVMs which pick the same SerialVersionUIDs, you can recover singletons. Everywhere else you were already in trouble anyway.

@scala-jenkins scala-jenkins added this to the 2.11.7 milestone Mar 31, 2015
@viktorklang
Copy link
Contributor

AFAIK if SerialVersionUID does not exist then one cannot guarantee sercompat even on a JVM-JVM basis?

@Ichoran
Copy link
Contributor Author

Ichoran commented Mar 31, 2015

@viktorklang - I am not sure how it works out in practice, but the spec does not describe in sufficient detail how to create the SerialVersionUID from the contents of a class. I'll try adding it to the abstract Infinity class and verify that it's propagated to the instances.

@viktorklang
Copy link
Contributor

You'll probably have to confirm Hotspot<->J9 unpatched vs patch

On Tue, Mar 31, 2015 at 7:09 PM, Ichoran notifications@github.com wrote:

@viktorklang https://github.com/viktorklang - I am not sure how it
works out in practice, but the spec does not describe in sufficient detail
how to create the SerialVersionUID from the contents of a class. I'll try
adding it to the abstract Infinity class and verify that it's propagated
to the instances.


Reply to this email directly or view it on GitHub
#4416 (comment).

Cheers,

@Ichoran
Copy link
Contributor Author

Ichoran commented Mar 31, 2015

@viktorklang - It doesn't matter; the SerialVersionUID doesn't propagate to the child classes. If we want this to be stable, we've got to force the compiler to emit a SerialVersionID for anonymous classes for the instances (which unless we change the build process takes two minor release cycles), or write serialization proxies for them (which seems like an awfully burdensome workaround). Or I just go fix everywhere where we try to eq it, and tell everyone else to use that method or wait for 2.12.

@viktorklang
Copy link
Contributor

I'd probably wait for 2.12 and recommend people not to use Java Serialization.

Made `Duration.Undefined`, `.Inf`, and `.MinusInf` all give back the singleton instance instead of creating a new copy by overriding readResolve.

This override can be (and is) private, which at least on Sun's JDK8 doesn't mess with the auto-generated SerialVersionUIDs.

Thus, the patch should make things strictly better: if you're on 2.11.7+ on JVMs which pick the same SerialVersionUIDs, you can recover singletons.  Everywhere else you were already in trouble anyway.
@Ichoran
Copy link
Contributor Author

Ichoran commented Mar 31, 2015

@viktorklang - Just realized that I could make the readResolve methods private which, while not a guarantee for success, at least allows this problem to be fixed without making anything worse. (SerialVersionUID is not, I think, supposed to depend on things that are invisible to the API.) We should still probably revisit this with a more robust solution in 2.12.

@viktorklang
Copy link
Contributor

Good idea

Cheers,

On 1 Apr 2015 00:43, "Ichoran" notifications@github.com wrote:

@viktorklang https://github.com/viktorklang - Just realized that I
could make the readResolve methods private which, while not a guarantee for
success, at least allows this problem to be fixed without making anything
worse. (SerialVersionUID is not, I think, supposed to depend on things that
are invisible to the API.) We should still probably revisit this with a
more robust solution in 2.12.


Reply to this email directly or view it on GitHub
#4416 (comment).

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

Successfully merging this pull request may close these issues.

4 participants