[threeten-dev] Some observations of chrono ser

[Xueming Shen]

On 03/15/2013 02:28 PM, roger riggs wrote:
> Ok, I see the point.
> Shall I sit my changes til we figure it out or commit and revisit?

Sure, you can commit and revisit later. I will try to pull in those
latest jdk8/master stuff after yours.

>
> (I updated the webrev. http://cr.openjdk.java.net/~rriggs/webrev-serialization-283/ )
>
> I think we settled the point to prevent built-in calendars from being overridden.
>
> I'm not sure whether the (loadable) Chronologies need to be treated as "trusted" or not.
> Some mischief can be made by lying about the time but the time basics that count
> would be ISO.
>
> Roger
>
> On 3/15/2013 5:13 PM, Xueming Shen wrote:
>> Roger,
>>
>> I have a "live" related bug/rfe #4619777 for a similar use scenario in my
>> java.nio.charset area. While the javadoc of CharsetProvider explicitly says
>> the context class loader will be used to load in the charset provider, our
>> real implementation actually uses system/application class loader. During
>> the discussion on whether we should change the implementation to fit the
>> spec, there is security concern raised from the vuln team that it is "normally"
>> a "bad" idea to use context class loader in this kinda scenario, given "the
>> thread context class loader is effectively a mutable static in an unconvincing
>> disguise, any trusted code trusting the ... would be vulnerable". We still have
>> not figured out the best solution for #4619777 yet (I understood the benefit
>> of using the context class loader and the possible limitation of using the
>> system/application class loader, but we may have to lean toward to change
>> the spec to fit with the implementation for compatibility and vuln concer),
>> I'm still thinking :-)
>>
>> The Chronology use scenario you are trying to achieve might be slightly
>> different, but it might need a second consideration.
>>
>> Another "similar" use case is the sun.util.locale.provider.SPILocaleProviderAdapter,
>> which also does not go with the context class loader, but these are all to
>> provider "global resource", maybe slightly different to the use scenario you're
>> trying to provide.
>>
>> -Sherman
>>
>> On 03/15/2013 12:38 PM, roger riggs wrote:
>>> 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
>>>
>>
>
> -- 
> 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
>