CODEWITHSUNDEEP
javastaticfindbugs
Charles
Charles

Reputation: 161

Best practice for updating/writing to static variable?

I have a project which displays department documentation. I store all the docs (get from database) in a static arrayList. Every X hour, I have that arrayList rebuilt based on the new doc (if any) from the database. There is also a static variable to control to rebuild that array or not, set and unset in a method which does the rebuild task. Each web browser hit the server will create this class's instance, but the doc arrayList and that control variable is shared among all the class instances.

Find-Bugs tool complains that "Write to static field someArrayName and someVariableName from instance method someClassMethod". Seems this is not the good thing to do (let class instance method write to static field). Does anyone have good recommendation how to get around this problem ? Thanks.

Upvotes: 5

Views: 16893

Answers (5)

Jason S
Jason S

Reputation: 189786

Per the FindBugs bug descriptions:

ST: Write to static field from instance method (ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD)

This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

Aside from the concurrency issues, it means that all of the instances in the JVM are accessing the same data, and would not allow two separate groups of instances. It would be better if you had a singleton "manager" object and passed it to each of the instances as a constructor parameter or at least as a setManager() method argument.

As for the concurrency issues: if you must use a static field, your static field should be final; explicit synchronization is difficult. (There are also some tricky aspects if you are initializing non-final static fields, beyond my knowledge of Java but I think I've seen them in the Java Puzzlers book.) There are at least three ways of dealing with this (warning, untested code follows, check first before using):

  1. Use a thread-safe collection, e.g. Collections.synchronizedList wrapped around a list that is not accessed in any other way.

    static final List<Item> items = createThreadSafeCollection();


static List<Item> createThreadSafeCollection()
{
   return Collections.synchronizedList(new ArrayList());
}

    and then later when you are replacing this collection, from an instance:

    List<Item> newItems = getNewListFromSomewhere();
items.clear();
items.add(newItems);

    The problem with this is that if two instances are doing this sequence at the same time, you could get:

    Instance1: items.clear(); Instance2: items.clear(); Instance1: items.addAll(newItems); Instance2: items.addAll(newItems);

    and get a list that doesn't meet the desired class invariant, namely that you have two groups of newItems in the static list. So this method doesn't work if you are clearing the entire list as one step, and adding items as a second step. (If your instances just need to add an item, though, items.add(newItem) would be safe to use from each instance.)

  2. Synchronize access to the collection.

    You'll need an explicit mechanism for synchronizing here. Synchronized methods won't work because they synchronize on "this", which is not common between the instances. You could use:

    static final private Object lock = new Object();
static volatile private List<Item> list;
// technically "list" doesn't need to be final if you
// make sure you synchronize properly around unit operations.


static void setList(List<Item> newList)
{
  synchronized(lock)
  {
      list = newList;
  }
}

  3. use AtomicReference

    static final private AtomicReference<List<Item>> list;


static void setList(List<Item> newList)
{
  list.set(newList);
}

Upvotes: 7

Tim Bender
Tim Bender

Reputation: 20442

If I understand the message you posted from Find Bugs correctly, this is just a warning.

If you want to hide the warning, do the modifications from a static method. Find Bugs is warning you because this is typically an error. The programmer thinks they are changing some instance state but really they are changing some state which impacts every instance.

Upvotes: 1

hvgotcodes
hvgotcodes

Reputation: 120258

You do not want to do this. Every request runs in its own thread. If the code that gets executed on a browser action modifies the list, then two requests can possibly modify the list at the same time, and corrupt the data. That is why it is not a good idea to access static resources from a non-static context, and probably why your tool is warning you.

Look at this

http://download.oracle.com/javase/6/docs/api/index.html?java/util/concurrent/package-summary.html

specifically the part about how the ArrayList is not synchronized. Also note that the paragraph I mention has a solution, specifically

List list = Collections.synchronizedList(new ArrayList(...));

Thats one way to do it. But its still not a good idea, namely because it can be slow. If its not a commercial-grade application, and you are not dealing in high volume, you can probably get by not making it better. If this is the type of app that only gets hit a few times per day, you can ignore the warning, with the understanding that its is possible that something bad will happen if two requests munge each other.

A better solution: Since you have database, I would just get the information from the db as you need it, i.e. as the requests come in. You can use some caching technologies for performance.

The reason I don't like the Singleton Pattern idea is that even if it makes the warning go away, it doesn't address the fundamental synchronization problem, by itself. There are thread safe http://en.wikipedia.org/wiki/Singleton_pattern#Traditional_simple_way_using_synchronization, however, which might work in this case.

Upvotes: -1

drekka
drekka

Reputation: 21883

You don't need to delete the list each time. As per above you will have to deal with multiple threads, but you can create the ArrayList once, then use clear() and addAll() methods to wipe and repopulate. FindBugs should be quite happy with that because you are not setting a static.

guys - feel free to chip in if there is any problem with this technique :-)

A second thought is to drive things from the database via hibernate. So don't maintain a list, hibernate has inbuilt caching so it's almost as quick. If you update the data at the database level (which means hibernate doesn't know) you can then tell hibernate to clear it's cache and refresh from the database when it's next queried.

Upvotes: 0

user541686
user541686

Reputation: 210603

Using the Singleton design pattern is one way. You can have only one instance of an object that holds the value you want, and access that instance through a global property. The advantage is that, if you want to have more instances later on, there's less modification of preexisting code (since you're not changing static fields to instance fields).

Upvotes: 0

Related Questions