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

Use public member instead of private field to resolve immutability #567

Merged
merged 1 commit into from
Jan 25, 2018

Conversation

MThomassen
Copy link
Contributor

Current com.esotericsoftware.kryo.Serializer::copy function determines immutability through its private field instead of it's public isImmutable() member (which docs actually mention). Since isImmutable() is virtual, this could break the copy api.

Note that this actually occurs in the Scala serializers provided by Twitter's Chill (Traversable.scala) and causes lot's of KryoException to be thrown in Apache Flink + Scala usage (one could argue the Twitter serializer implementations are to blame.. or chaning member isImmutable() to be final would be a preferred option, but that would break API compatibility)

@isaki
Copy link
Contributor

isaki commented Jan 14, 2018

The immutable field is set in the constructor so this isn't really needed; it is arguable that the copy method should have been marked final, but given there are people who have overridden it, this might be the only solution if for some reason you don't want to set it in the constructor.

@NathanSweet
Copy link
Member

It's typical in Java for methods to not be marked final and anyone overriding a method, especially a getting, should be wary that the method may not be used internally. The javadocs here are on the setter, I'm not sure why anyone would think it a good idea to override the getter. Using only public accessors internally is generally a pain, though this particular scenario it is very simple, so I'm OK with it. In other areas, it's not safe to assume getters will be used internally.

@NathanSweet NathanSweet merged commit fa19b4c into EsotericSoftware:master Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants