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 and deserialization of hash-based collections should not re-use hashCode #1600

Closed
scabug opened this issue Dec 24, 2008 · 11 comments
Assignees

Comments

@scabug
Copy link

scabug commented Dec 24, 2008

As explained on ticket #1387, the fact that the hashCode for tuples and case classes depends on the VM (they use getClass().hashCode()) means that the serialization and deserialization of immutable.HashSet (and probably other hash-based collections, but I have not verified) breaks.

Although I think it's worth fixing tuples and case classes, this issue is about changing hash-based collections so that serialization and deserialization work correctly even if the hashCode relies on some VM-specific computation.

The easiest way to do that is similar to how java.util.HashMap does it by serialising the objects into an array and then inserting them one by one into the Map on deserialisation. This means that the hashCodes are recomputed on deserialisation so there are no issues anymore.

@scabug
Copy link
Author

scabug commented Dec 24, 2008

Imported From: https://issues.scala-lang.org/browse/SI-1600?orig=1
Reporter: @ijuma
Attachments:

  • t1600.diff (created on Nov 30, 2009 7:04:50 PM UTC, 20566 bytes)

@scabug
Copy link
Author

scabug commented Jan 4, 2009

@odersky said:
I don't want to fix this myself. Any other takers?

@scabug
Copy link
Author

scabug commented Jan 5, 2009

@ijuma said:
I could supply a patch, but there are two questions:

  • What are the backwards compatibility requirements?
  • Is it worth doing this now, or better to do it for the new collections in 2.8.0?

@scabug
Copy link
Author

scabug commented Jan 5, 2009

@odersky said:
I think it's better to wait for 2.8.0, which will have looser backwards requirements as well. I'd happily integrate a patch then!

@scabug
Copy link
Author

scabug commented Nov 29, 2009

@ijuma said:
I committed this to a github branch:

http://github.com/ijuma/scala/commit/b7b0459ae872701a9486d7692b76a353b02cbfbc

I will also attach a patch.

Commit comment follows for convenience:

Fix ticket #1600: Serialization and deserialization of hash-based collections should not re-use hashCode.

Implementation and tests are included for immutable.HashMap, mutable.HashMap, mutable.LinkedHashMap, immutable.HashSet, mutable.HashSet, immutable.LinkedHashSet.

The basic idea is that we store the size, load factor and elements during serialization and we rebuild the collection on deserialization. Note that this is not compatible with the previous serialization format. All @SerialVersionUIDs have been reset to 1.

I'm not particularly happy about the way _loadFactor is used in some cases, but that seemed like the lesser evil given the current interface.

WeakHashMap is not Serializable and it would not make sense to make it so. TreeHashMap has not been reintegrated yet. OpenHashMap has not been updated. I think that collection is flawed in many ways and it should either be removed or it should be reimplemented. I'll file a separate ticket for that.

Some tests in serialization.check were updated as the toString order after serialization can be different in some cases.

ant all.clean && ant dist finished successfully.

@scabug
Copy link
Author

scabug commented Nov 29, 2009

@scabug
Copy link
Author

scabug commented Nov 30, 2009

@ijuma said:
Implementation and tests for this issue.

@scabug
Copy link
Author

scabug commented Nov 30, 2009

@ijuma said:
Rebased the patch and github branch and improved documentation:

http://github.com/ijuma/scala/commit/733eec09d25c0580ef65a32945b053da6c2d87e8

Assigning to scala_reviewer as suggested by Toni.

@scabug
Copy link
Author

scabug commented Nov 30, 2009

@adriaanm said:
Thanks for the patch!

@scabug
Copy link
Author

scabug commented Dec 1, 2009

@adriaanm said:
Paul, could you please shepherd this?

@scabug
Copy link
Author

scabug commented Dec 1, 2009

@ijuma said:
Paul committed this in r19964. Thanks, closing.

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

No branches or pull requests

2 participants