[threeten-dev] Some observations of chrono ser

[Roger Riggs]

Hi Sherman,

Sorry for the race condition, I was trying out a few things...

On 3/14/13 3:33 PM, Xueming Shen wrote:
> On 03/14/2013 10:42 AM, roger riggs wrote:
>> Hi,
>>
>> On 3/14/2013 11:40 AM, Xueming Shen wrote:
>>> On 3/14/13 7:29 AM, roger riggs wrote:
>>>> Hi  Sherman,
>>>>
>>>> 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

>
> -Sherman
>
>