JDK 9 RFR of JDK-8035452: Fix serial lint warnings in core libs

[Joe Darcy]

On 02/24/2014 10:53 PM, Stuart Marks wrote:
> On 2/24/14 8:22 PM, Joe Darcy wrote:
>> On 02/20/2014 12:49 PM, Paul Benedict wrote:
>>> Joe, I find it interesting that you suppressed the serial warning on an
>>> abstract class. I'd like to know more about that. Is this a 
>>> universal rule?
>>>  Are serial uids not important for abstract classes?
>>
>> I wouldn't generalize that way from this example.
>>
>> The serial hash of NavigableSubMap has differed in different JDK 
>> releases,
>> but its subclasses do define serialVersionUID fields. I assume the 
>> set of
>> non-transient fields in NavigableSubMap has stayed unchanged, but I 
>> haven't
>> verified that.
>>
>> If I hadn't found the change in the hash value, I would have added the
>> serialVersionUID to NavigableSubMap too.
>
> And in his reply to Paul, Joe said:
>> From what I was able to discern by reading the serialization 
>> specification
>> [1], If a class does *not* declare a serialVersionUID, the 
>> serialVersionUID
>> of its superclass is *not* included in the serialver hash computation 
>> of the
>> child class. However, my understanding is that changes to the fields 
>> stored
>> in superclass and changes to the semantics of its readObject / 
>> writeObjects
>> methods could affect the serialization of the child class.
>
> I think we need to take a closer look at these issues.
>
> I believe that abstract, serializable superclasses *do* need to have a 
> serialVersionUID defined. The reason is that when a subclass is 
> serialized, its superclass descriptor (an ObjectStreamClass) is also 
> serialized. Upon deserialization, the descriptor's svuid is matched 
> against the svuid of the class loaded at runtime, and if there is a 
> mismatch, InvalidClassException ensues.
>
> While the svuid of an abstract superclass isn't included in the 
> subclass' svuid hash, the svuid of the superclass does affect serial 
> compatibility of subclasses as described above. Thus, an apparently 
> innocuous change to the superclass might prevent serial compatibility 
> of its subclasses, no matter how carefully the subclasses are programmed.
>
> If the NavigableSubMap class has changed svuid values over several 
> releases, well, unfortunately we may have a compatibility problem 
> already in the field. We'd need to choose which release to be 
> compatible with. Since 8 isn't quite out yet, we might be able to 
> change an early 8-update and 9 to be compatibile with the latest 
> 7-update.
>
> Note that the svuid of a class does not relate solely to the fields 
> that are serialized. It's an attempt at a version hash of the 
> *implementation* of a class, not a version of the serial protocol. 
> Even changes to a class that don't affect the serialized output stream 
> can affect the svuid. For example, renaming a package-private method 
> will affect the svuid. See section 4.6 of the serialization spec.

I am trying hard to remain blissfully ignorant of any more low-level 
details of the serialization format; however, I might not be successful 
on that goal much longer ;-)

My preference in a case like this is to add the svuid if for no other 
reason that is is simple to explain and understand, even if it is not 
strictly required.

> While we're at it (sorry...) in the diffs for your other serial 
> warnings patch JDK-8035453, there are several lines where the serial 
> warning is suppressed like so:
>
> + at SuppressWarnings("serial") // JDK implementation class
>
> As you know, serialization can expose the private fields of a class, 
> making them public in a sense. Serialization can also expose what are 
> internal, implementation classes, if these classes are part of a 
> serializable object graph that is exposed to applications. I don't 
> know about the specific situation with the DOM classes, but even if a 
> serializable class is internal, we might need to be concerned about 
> serialization compatibility.

There is a difference in character between a serializable class in Java 
SE (java.* and javax.*) and the jdk.Exported(true) types in the JDK and 
a serializable class that lives in sun.* or some other 
jdk.Exported(false) area.

For that latter, the serialization contract has to be different, with 
fewer guarantees, just as the general usage contract for those types has 
fewer guarantees. I think this is analogous to putting non-serializable 
classes into collections; the collection itself is serializable, but it 
won't be anymore if you put non-serializable objects into it.

If a user happens to have a direct or indirect reference to an object of 
a JDK implementation type, the compatibility contract is weaker than if 
an object with a public Java SE type were being dealt with.

>
> Finally, EnumSet doesn't need a serial version UID. It's serialized 
> using a proxy class, so EnumSet never appears in a serialized byte 
> stream. (Note, its readObject throws an exception unconditionally.) So 
> it's probably safe to suppress its serialization warning.
>

Yes, EnumSet was a bit tricky, it is serializable itself, but uses a 
proxy internally. ("Effective Java, 2nd edition" both recommends the 
proxy pattern and recommends adding a svuid to all serializable classes, 
but doesn't explicitly give guidance to this combination of features.)

To avoid adding a long comment explaining the proxy pattern and why a 
svuid on EnumSet isn't really required, my preference would just be to 
add the svuid if it doesn't cause any harm.

Thanks,

-Joe