1

I have a library that I am required to use that has a dangerous initialization of a static value (the classes have been stripped down to the minimum for the example):

public TheirBaseClass {
    public static String PathToUse = null;

    public BaseClass(){
        PathToUse = "Configured";

        // ... 
        // do some other stuff with other side effects 
        // ...
    }
}

I have a case where I attempt to read from the value ConfigValue without instantiating the class (to avoid some of the sideeffects).

Paths.get(TheirBaseClass.PathToUse).toFile()....

This causes a NullPointerException

Because I am required to use this class, I am looking to inherit from it, and attempt to take action to ensure that initialization has taken place when accessing the static.

public MyBaseClass extends TheirBaseClass{

    private static final AtomicBoolean isInitialized = new AtomicBoolean(false);

    static {
        MyBaseClass.Initialize();
    }

    public static void Initialize(){
        // FindBugs does not like me synchronizing on a concurrent object
        synchronized(isInitialized){
            if( isInitialized.get() ){
                return;
            }
            new TheirBaseClass();
            isInitialized.set(true);
        }
    }

    public MyBaseClass(){
        super();
    }

}

Which allows me to 

MyBaseClass.Initialize();
Paths.get(MyBaseClass.PathToUse).toFile()....

This seems to be working reasonably well (and resolves some other phantom defects we've been having). It allows TheirBaseClass to function naturally, while allowing me to safely force initialization in the couple of cases I may need to.

However when I run FindBugs against this code, I get JLM_JSR166_UTILCONCURRENT_MONITORENTER. After reading the description, I agree that the use of AtomicBoolean could be dangerous because someone else could change the value, but...

  1. I think its safe to ignore in this case (but have a doubt)
  2. I generally prefer to rewrite code than put an ignore marker in place

Am I actually doing something dangerous (and just don't see it)? Is there a better way to do this?

Unfortunately, using a different TheirBaseClass is not an option.

Related

Community
  • 1
  • 1
Jefferey Cave
  • 2,507
  • 1
  • 27
  • 46

1 Answers1

2

You might find it easier to adapt the lazy holder idiom:

public MyBaseClass {
  private static class Initializer {
    static {
      new TheirBaseClass();
    }

    // Doesn't actually do anything; merely provides an expression
    // to cause the Initializer class to be loaded.
    private static void ensureInitialized() {}
  }

  {
    Initializer.ensureInitialized();
  }

  // Rest of the class.
}

This uses the fact that class loading happens only once and is synchronized (within a single class loader). It happens only when you instantiate a MyBaseClass.

Andy Turner
  • 137,514
  • 11
  • 162
  • 243
  • I think I made a massive mistake in my example: I didn't extend `TheirBaseClass` – Jefferey Cave Aug 25 '16 at 18:23
  • I had to ponder this. I am already implementing `lazy holder`. Your answer is correct, but the confusion seems to arise around the boolean: it is completely unecessary. I introduced that for a specific case, but that case seems to have gone away with more and more refactoring. – Jefferey Cave Aug 30 '16 at 13:57
  • The boolean is unnecessary *if I make the initializer private* – Jefferey Cave Sep 02 '16 at 12:17