[threeten-dev] Some observations of chrono ser

[roger riggs]

Ok, I see the point.
Shall I sit my changes til we figure it out or commit and revisit?

(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