<i18n dev> DateFormatSymbols triggers this.clone() in the constructor

[Naoto Sato]

Okutsu-san, Umaoka-san,

OK I got it now. We should cache data fields instead of the object itself.

Naoto

On 6/7/15 10:33 PM, Masayoshi Okutsu wrote:
> I think what Umaoka-san pointed out is valid. Overrideable methods
> shouldn't be called in a constructor.
>
> Masayoshi
>
> On 6/6/2015 6:53 AM, Naoto Sato wrote:
>> Umaoka-san,
>>
>> I believe the cloning is needed to return the defensive copy,
>> otherwise an app can mutate the state of the DateFormatSymbols for
>> other apps. One solution could be to not cache the instance if its
>> from SPI, but apparently that will affect the performance. I don't see
>> anything wrong to check if foo is null or not in clone(), since it's a
>> private property to the provider.
>>
>> Naoto
>>
>> On 6/5/15 8:31 AM, Yoshito Umaoka wrote:
>>> Hi folks,
>>>
>>> ICU4J implements Java locale service provider interface. I recently
>>> received a problem report from our customer. When they upgraded Java
>>> version to 8, the provider implementation stopped working because of NPE
>>> thrown by our custom DateFormatSymbols subclass. I dug into the problem
>>> and found that DateFormatSymbols caches its own instance in the
>>> constructor. Our subclass implementation expects a field is always
>>> initialized to non-null in the constructor. However, clone() method is
>>> called for the super class's constructor, the field is not yet
>>> initialized at the point.
>>>
>>> It looks adding 'null' for the field in clone() method would resolve the
>>> immediate problem, but I'm not comfortable that DateFormatSymbols
>>> implementation cache a premature instance of my own subclass. If any
>>> methods overridden by my own subclass is called on a premature instance,
>>> it might cause another issue.
>>>
>>> Anyway, I filed a bug in the Java bug database with the description
>>> below. For meanwhile, adding simple 'null' check in our code would
>>> suffice. But I would like to hear Java i18n team's opinion about this
>>> issue.
>>>
>>> Thanks,
>>> Yoshito
>>>
>>>
>>> ---------------------------
>>> To implement my own locale service provider, I have a class extending
>>> java.text.DateFormatSymbols. My custom subclass's constructor implicitly
>>> invokes the no-args constructor in java.text.DateFormatSymbols. The
>>> constructor calls a private method - initializeData(Locale).
>>>
>>> It looks the implementation was updated in Java 8 and initializeData is
>>> now trying to cache an instance of DateFormatSymbols at the end and
>>> calls this.clone().
>>>
>>> In my own subclass implements clone() method, which copies a field
>>> initialized by a constructor in the class. For example,
>>>
>>> ===============
>>> public class MyDateFormatSymbols extends DateFormatSymbols {
>>>      private final Foo foo;
>>>
>>>      public MyDateFormatSymbols(Foo foo) {
>>>          if (foo == null) {
>>>              this.foo = new Foo();
>>>          } else {
>>>              this.foo = foo;
>>>          }
>>>      }
>>>
>>>      @Override
>>>      public Object clone() {
>>>          MyDateFormatSymbols mdfs = (MyDateFormatSymbols)super.clone();
>>>          mdfs.foo = this.foo.clone();
>>>      }
>>> }
>>> ===============
>>>
>>> When the constructor MyDateFormatSymbols(Foo) is called, it triggers
>>> no-args constructor of the super class - DateFormatSymbols() first. As I
>>> explained earlier, Java 8 implementation calls this.clone() in
>>> DateFormatSymbpls.initializeData(Locale). At that point, the field foo
>>> in my class is not yet initialized, so this.foo.clone() will throw
>>> NullPointerException.
>>>
>>> My own code expects the field 'foo' is always non-null. I could change
>>> clone() to check if this.foo is null or not, but I cannot control cached
>>> 'premature' instance held by Java DateFormatSymbols. At this moment, it
>>> looks the cache is only used for copying field values maintained by
>>> DateFormaSymbols itself and never call a method. So, even 'premature'
>>> instance of my own subclass instance is referenced by DateFormatSymbols,
>>> it won't cause any problems. However, if the Java's implementation is
>>> changed to call any DateFormatSymbols method overridden by my own
>>> subclass, it may not work (because my subclass expects the field foo is
>>> non-null).
>>>
>>> Such code above did not have any problems with earlier Java releases
>>> (Java 6 / 7).
>>>
>>> In my opinion, this.clone() should not be called in DateFormatSymbols
>>> initialization code. Instead, it should create a private container class
>>> for these symbols, and cache the object, not DateFormatSymbols itself.
>