[threeten-dev] Some observations of chrono ser

[Xueming Shen]

The change itself looks fine for me, however I have couple more questions
regarding the Chronology spec and implementation :-)

(1) it appears, compared to Chronology.of(id), the Chronology.ofLocale(locale)
has a slightly different implementation for looking up and loading, it stops after
the look up of CHRONOS_BY_TYPE (built-in and application level service loader),
while the Chronology.of(id) continues go on for the possible candidates from
the ServiceLoad(contextClassLoader). Again, is this intended?

(2) while I understand the intent of looking up a Chronology implementation
via contextClassLoader, but it appears you can then pass around such a context
specific Chronology (and probably the corresponding ChronoDate) around and
to be shared beyond the original thread's "context", is this desirable? or we go
with the enforcement of any Chronology is a global resource, need to be initialized
via the application/system class loader?

-Sherman

On 03/15/2013 11:01 AM, roger riggs wrote:
> 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
>>