[threeten-dev] Some observations of chrono ser

[roger riggs]

Hi Sherman,

On 3/15/2013 3:08 PM, Xueming Shen wrote:
> 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?
That is an oversight.
>
> (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?
With or without the help of Chronology an application can load
a Chronology via the ServiceLoader and it should be usable.
Regardless of where the application passes the reference it will
continue to provide it own implementations of the Chrono* interfaces.

Using the ContextClassLoader is a convenience and gives a consistent API for
locating Chronologies. Using the ContextClassLoader appears to be the
default for ServiceLoader.load so I don't see an issue there. Whether
a client or server, it should find the Chronology in the provided context.

Is there anything similar in other APIs;  ResourceBundles for example?

Roger

>
> -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
>>>
>

-- 
Thanks, Roger

Oracle Java Platform Group

Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
developing practices and products that help protect the environment