Best practice for updating / writing a static variable?

Question

I have a project that displays department documentation. I store all documents (retrieved from the database) in a static list of arrays. Every X hours I have that arrayList is rebuilt based on a new document (if any) from the database. There is also a static control variable to rebuild this array or not, set and cancel in a method that performs the rebuild task. Each web browser that hits the server will create this instance of the class, but the doc arrayList and this control variable will be shared by all instances of the class.

The Find-Bugs tool complains that "Write to static field someArrayName and someVariableName from the instance method someClassMethod". This does not seem to be very good (let the class instance method write to a static field). Anyone have a good recommendation on how to get around this? Thanks.

Answer 1

In the FindBugs error descriptions :

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

This instance method writes to the static field. This is difficult to do correctly if you manipulate multiple instances and, as a rule, are bad practice.

Besides concurrency issues, this means that all instances in the JVM access the same data and do not allow two separate groups of instances. It would be better if you had one "manager" object and passed it to each instance as a constructor parameter, or at least as an argument to the setManager() method.

Regarding concurrency issues: if you should use a static field, your static field should be final; explicit synchronization is difficult. (There are some difficult points if you initialize unfinalized static fields besides my knowledge of Java, but I think I saw them in the Java Puzzlers book.) There are at least three ways to deal with this (warning, unverified code follows, first check before use):

• Use a thread safe collection , for example. Collections.synchronizedList wrapped around a list that is not accessible in any other way.

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

  and then later when you replace this collection from the instance:

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

  The problem is that if two instances execute this sequence at the same time, you can get:

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

  and get a list that does not match the desired class invariant, namely that you have two groups of new elements in a static list. Thus, this method does not work if you clear the entire list as one step and add items as the second step. (If your instances just need to add an item, however, items.add(newItem) will be safe to use from each instance.)

• Synchronize access to the collection.

  For synchronization, you will need an explicit synchronization mechanism. Synchronized methods will not work because they are synchronized to "this", which is not common between instances. You can 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; } } 

• use atomicreference

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

Answer 2

If I correctly understood the message sent using "Find Errors", this is just a warning.

If you want to hide the warning, make changes from the static method. Finding errors warns you because it is usually a mistake. The programmer believes that they change the state of the instance, but in fact they change the state that affects each instance.

Answer 3

Using the Singleton design template is one way. You can have only one instance of an object that contains the desired value, and access this instance through a global property. The advantage is that if you want to have more instances later, there are fewer changes to the existing code (since you do not change the static fields into the instance fields).

Answer 4

You do not need to delete the list every time. As described above, you will have to deal with multiple threads, but you can create an ArrayList once, and then use the clear () and addAll () methods to clear and refill. FindBugs should be very happy with this because you are not installing a static file.

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

The second thought is to bring things out of the database through sleep mode. So do not maintain the list, sleep mode has built-in caching, so it is almost as fast. If you are updating data at the database level (which means hibernate does not know), you can tell hibernate to clear the cache and update it from the database the next time you query.

Answer 5

You do not want to do this. Each request is launched in its own thread. If the code that is executed in the browser action changes the list, then two requests can simultaneously change the list and corrupt the data. This is why it is not recommended 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

in particular, the part on how an ArrayList is not synchronized. Also note that in paragraph I mention the solution, in particular

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

This is one way to do this. But this is still not a good idea, namely because it can be slow. If this is not a commercial application, and you are not dealing with a large volume, you can probably get it without making it better. If this is a type of application that only hits a few times a day, you can ignore this warning, realizing that it is possible that something bad will happen if the two requests go to each other.

Best solution: since you have a database, I just get information from db as needed, i.e. as requests arrive. You can use some caching technologies for performance.

The reason I don't like the idea of ​​the Singleton Pattern is that even if this makes the warning go away, it does not in itself affect the underlying synchronization problem. However, there are thread-safe http://en.wikipedia.org/wiki/Singleton_pattern#Traditional_simple_way_using_synchronization that may work in this case.