[threeten-dev] Some observations of chrono ser

[Xueming Shen]

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
>