What's the best way to fix this 'write to static field from instance method' findbugs warning?

Question

I have a class that looks similar to this, and findbugz is complaining about the 'write to the static field from the instance method' (initialize(), and killStaticfield()). I can't set the static field in the ctor.

• What is the best fix for this issue?

• Would putting staticField in an AtomicReference suffice?

   public class Something
   {
    private static SomeClass staticField = null;
    private AnotherClass aClass;
    public Something()
    {
  
    }
  
    public void initialize()
    {
      //must be ctor'd in initialize
      aClass = new AnotherClass();
      staticField = new SomeClass( aClass );
    }
  
    public void killStaticField()
    {
     staticField = null;
    }
  
    public static void getStaticField()
    {
      return staticField;
    }
  }
  

Sassy

To answer your question, this field is static because the get method needs to be static so other objects can access the staticField without having a reference to a Something object. – Sassy

Basically, my question is what's the best way to fix the 'write to static field from instance method' findbugz warning; I just made up the code to represent the warning. Is it better to wrap the static object in an AtomicReference object, or to synchronize? – Sassy

Answer 1

Staying as close as possible to your original design...

public class Something {
  private static volatile SomeClass staticField = null;

  public Something() {
  }

  public static SomeClass getStaticField() {
    if(Something.staticField == null)
      Something.staticField = new SomeClass();;
    return Something.staticField;
  }
}

Refer to your static variable via the class name, that will remove the findbugz warning. Mark your static variable as volatile, which will make the reference safer in a multithreaded environment.

Even better would be:

public class Something {
  private static final SomeClass staticField = new SomeClass();

  public Something() {
  }

  public static SomeClass getStaticField() {
    return Something.staticField;
  }
}

Subtreasury

But a design without synchronized would also mean no killStaticField. – Avraham

You know... Looking at the first solution, its still not thread-safe, even though the variable is volatile. You'd have to put a synchronized{} block around the if-null-then-new section... I'd definitely prefer the second option. – Subtreasury

I think (unfortunately) the first solution is the right answer in my case. Mainly because I can't initialize staticField at the top, it has to be initialized after initialize() is called (I'll edit the question to reflect that). – Sassy

If we went with the 1st code block we'd have to make all access to staticField synchronize around SomeClass.class. – Sassy

I changed its reference by using ClassName.staticField, however findbugs still complain the same thing. – Ruysdael

Answer 2

The question is what you want to do with the static field. If it changes for every class you create it might not be a good idea to have it static at all. If it gets initialized only once you should just lazily initialize it as a singleton.

public class Something
{
    private static SomeClass staticField = null;

    public Something()
    {

    }

    public static SomeClass getStaticField()
    {
        if(staticField == null)
            staticField = new SomeClass();;
        return staticField;
    }
}

Valida

Don't forget to make that getStaticField method synchronized! – Broker

And note, you can't just put synchronized on the static method definition, as static methods use the Something.class to synchronize against! – Sassy

I think the use of static variables should be clear before discussing any advanced threading issues. And sorry, but your question doesn't really sound as if you understood when to use static variables and when not. Maybe you can clarify your question a little more. – Valida

@Sassy What is wrong with syncing on the class? If your class is only a singleton holder (as your example) that is a decent sync solution. – Avraham

Answer 3

Remove static from staticField if it should not be static.

Make kill and getStaticField static themselves. And you usually reference static by the class name, not by an (implicit) this, to make very clear that it is static and may cause unexpected consequences in other thReads.

When in doubt, don't use statics for non-constant fields.

Avraham

You don't nee the "When in doubt". – Stretcherbearer

Answer 4

The best way is not to do it, try to find a better design patern. If really necessary this will work and make findbugs/spotbugs not complain.

public class Something
{
    private static SomeClass staticField = null;

    public Something()
    {
    }

    private void setStaticField(SomeClass value){
        staticField=value;
    } 

    public static SomeClass getStaticField()
    {
        if(staticField == null)
            setStaticField(new SomeClass());
        return staticField;
    }
}

Boley