Jefferey Cave Jefferey Cave - 3 months ago 15
Java Question

is FindBugs 'JLM_JSR166_UTILCONCURRENT_MONITORENTER' safe to ignore in this case

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.

Answer

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.

Comments