[threeten-dev] Some observations of chrono ser

[roger riggs]

Hi,

Updated the webrev with removal of unnecessary/unused readResolve methods,
reordered the Chronology.initCache method to ignore duplicate Chronologies
from ServiceLoader.

Webrev: http://cr.openjdk.java.net/~rriggs/webrev-serialization-283/

Included a test fix for the JapaneseDate.of(1,1,1) problem.

Roger


On 3/14/2013 9:07 PM, Roger Riggs wrote:
>
>>>>>
>>>>> On 3/12/2013 5:52 PM, Xueming Shen wrote:
>>>>>> Hi,
>>>>>>
>>>>>> (1) Chronology.writeReplace() is private, so the proxy Ser mechanism
>>>>>> is actually disabled for all Chronology subclasses. Is it 
>>>>>> intentional? Given
>>>>>> all JDK subclasses all have readResolve() defined (3 out of 4 to 
>>>>>> return
>>>>>> the singleton) I would assume the Ser delegation might not be 
>>>>>> necessary,
>>>>>> though it might be still desirable to "enforce" the Ser pattern 
>>>>>> here?
>>>>> Not quite, but still broken.  The writeReplace method can be 
>>>>> private but
>>>>> is used only if the Chronology class is Serializable.
>>>>> http://docs.oracle.com/javase/7/docs/platform/serialization/spec/output.html#5324 
>>>>>
>>>>>
>>>>> The subclasses are serializable and that is triggering the default 
>>>>> serialization.
>>>>>
>>>>> Is there an issue for this?
>>>>
>>>> I think a private writeReplace method don't have an effect on how 
>>>> its serializable
>>>> subclass serializes itself. So I believe the existing 4 jdk 
>>>> Chronology subclasses
>>>> are using the "jdk default mechanism" (not the Ser 
>>>> delegation/proxy), so I was
>>>> asking if it is an intentional design/implementation. For any other 
>>>> potential
>>>> user defined Chronology subclass, does the abstract Chronology have 
>>>> anything
>>>> really need to be "serializable" and via the Ser mechanism? Two 
>>>> possible designs
>>>> here
>>>>
>>>> (1) we want all the Chronology subclasses to be serialized via 
>>>> "Ser" (then it can
>>>> be done via the Chronology.of(...) when read in), then the 
>>>> writeReplace() probably
>>>> should be declared as non-private?
>>> The intent was that Chronology would always handle serialization
>>> and return the same (singleton) instances that are known to Chronology.
>>>
>>> To make that work the writeReplace method needs to be "protected final"
>>> and visible to subclasses and not be override-able.
>>>
>>
>> Something like the attached webrev?
>>
>> http://cr.openjdk.java.net/~sherman/jdk8_threeten/serChrono
> Yes, same as what I did.
>>
>> Do we really to make it "final" to enforce the abstract base class 
>> Chronology to
>> always handle the serialization? Or be liberal to document that this 
>> is encouraged,
>> but not enforced, with a "non-final" Chronology.writeReplace()
>
> I went with non-final because as final it would prevent subclassing a 
> Chronology
> that had its own state to serialize, like the HijrahChronology that 
> has variants.
> It seems inflexible to prevent that from anything but bundled 
> Chronologies.
>>
>> Second question leads to my (2) in previous email. With the 
>> enforcement of
>> Chronology.writeReplace()
>>     ->Ser
>>         ->Chronology.readExternal()
>>             ->Chronology.of/of0(id)
>>                 ->cache.
>>
>> A Chronology instance may be serialized from a vm default impl to a 
>> user-defined
>> Chronology in other vm, if there is overriding Chronology 
>> implementation, however,
>> a built-in ChronoLocalDate, for example JapaneseDate, is always 
>> hard-wired to the
>> built-in JapaneseChronology, so if you serialize and de-serialize a 
>> JapaneseChronology
>> AND a JapaneseDate into other vm, you may get a user-defined 
>> JapaneseChronology
>> and a built-in JapneseDate with bundled with a built-in 
>> JapaneseChronology, something
>> undesired?
> I suppose there is little benefit from allowing the built-in 
> chronologies to
> be overridden and it does open up some potential problems.
> To insure consistency, JapaneseDate could use 
> Chronology.of("Japanese") to get
> the Chronology but that adds very little and will be slower.
>
> If we do away with the override then there should be at least a log
> message if the register of the ServiceLoader chronology fails.
>
> Also, note that the synchronization on whether initialization is complete
> is based on finding ISOChronology in the HashTable.  That would need
> a different mechanism. (Its ok for multiple threads to do the init 
> concurrently
> only one will win and it avoids a single threaded synchronization.)
>
> Roger
>