Sato-san,
Yes, it's not thing wrong to check null in the clone() method, and
that's what I did to avoid this problem. But what we can do there is to
skip cloning the field in our code only.
With the current Java implementation, I'm pretty sure that it won't
cause any actual problem, because Java implementation does not call any
methods on the cached copy. The implementation only reads previously
initialized symbols (such as weekday name, month names) by directly
accessing the fields.
However, if the implementation is changed in future and calls any
DateFormatSymbols' public method, our subclass won't work because our
class overrides these methods and expects the field (which is supposed
to be initialized by our class's constructor) is null.
I'm not suggesting that Java DateFormatSymbols should not utilize a
cache storing symbols initialized from resource bundles. My point is -
caching an instance of DateFormatSymbols itself could be dangerous
because of the reason I explained above.
I think the right thing to do is to create a private class object
holding these symbols (this can be a singleton per Locale) separate from
DateFormatSymbols - and DateFormatSymbols constructor just copy the
field to a new DateFormatSymbols 2nd time.
For example, creating a private class:
private static class SymbolsCacheEntry {
String eras[];
String months[];
....
String patternChars;
....
}
Then actual locale data is once loaded into SymbolsCacheEntry - and
cached. When DateFormatSymbols constructor is called 2nd time,
"copyMembers()" copies field values from an instance of
SymbolsCacheEntry (instead of cached DateFormatSymbols), that is,
private void copyMembers(SymbolsCacheEntry src, DateFormatSymbols dst) {
dst.eras = Arrays.copyOf(src.eras, src.eras.length);
....
}
By doing so, the implementation can avoid premature (and potentially
dangerous) DateFormatSymbols subclass instance.
Anyway, if Java guarantees that the implementation will never call
DateFormatSymbols methods (such as getMonths()) in future and just
directly accessing Java's DateFormatSymbols' own fields, then I'm OK
with it. (In this case, I would suggest Java team to add some comments
in the source code, so future maintainer of the Java code can understand
the risk.)
Thanks,
Yoshito
On 6/5/2015 5:53 PM, 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.
