2

Let there be a class definition like

  public static class Bootstrapper {

    public static final Object DEFAULT_VALUE = getDefaultValue();

    private static Object getDefaultValue() {
      if (DEFAULT_VALUE == null) {
        return createValue(); // Not thread safe
      }
      return DEFAULT_VALUE;
    }
  }

where the createValue() method does not reference the DEFAULT_VALUE field, is only otherwise called in the constructor of the Bootstrapper class and is not thread safe.

Is there any issue (aside from programming style) with the above code? Presumably thread safety is not a problem, given the rules for class initialization, but anything important for the programmer to be aware of?

PNS
  • 19,295
  • 32
  • 96
  • 143
  • i am not quite sure, but i think in such context, `getDefaultValue` will only get called once, right before the first time you reference this class. – Jason Hu Mar 04 '15 at 21:35
  • generally speaking, if you want thread safe, just go ahead and add `synchronized` – Jason Hu Mar 04 '15 at 21:37
  • Agreed, but I came across similar code like the above and I am curious as to what actually happens in such situations. – PNS Mar 04 '15 at 21:40
  • class loader will load your class, and then initialize your static variable first, then call static constructor. if my memory is correct, this is thread safe. it's reasonable, since jvm needs to guarantee the class only get loaded once and only once. – Jason Hu Mar 04 '15 at 21:42
  • Sure, but imagine a scenario in which the `createValue()` method references a static singleton object, also used by other code, while the `Bootstrapper` class is being loaded, or even using different classloaders. It looks like the `synchronize` keyword is in order here for the `createValue()` method. – PNS Mar 04 '15 at 21:45
  • if you are saying there are resources beyond the scope of this class and accessed in multi-threaded environment, there is nothing to do with this class or method now. all critical section needs to be protected, it's the rule. the `synchronized` is used to protect the data. since you are not exposing any data, it's hard to tell whether it's good or not. – Jason Hu Mar 04 '15 at 21:49

2 Answers2

3

As Augusto explains, your code is thread-safe. But it's rather convoluted. It would be functionally equivalent, slightly more efficient, and much clearer to simply do this:

   public static class Bootstrapper {    
     private static final Object DEFAULT_VALUE = createValue();

     public static Object getDefaultValue() {
       return DEFAULT_VALUE;
     }
   }

Edit: I also just noticed that the field was public and the getter was private. That should probably be the other way around.

Kevin Krumwiede
  • 9,868
  • 4
  • 34
  • 82
2

This is safe from a threading point of view, as the class loading is thread safe and that value will be set (so getDefaultValue()) will be called after the class is loaded, but before it leaves the class loading code.

To answer PNS comment on the original question above, if the class is loaded by 2 different classloaders you are in trouble anyway, as using the synchronized keyword on getDefaultValue() will create a lock on the class... and since you have 2 classes, each one will be fully independent. You can read this in the Java Language Specification, section 4.3.4 When Reference Types Are the Same (for JLS 8).

Augusto
  • 28,839
  • 5
  • 58
  • 88