Protecting a static class variable

Question

I have a fairly trivial static variable question. I'm building out a solution that loosely follows the path or RMI. On my server, I have a ComputeEngine class that will execute 'Tasks' (class instances with an 'execute' method). However, the ComputeEngine will contain a global variable that will need to be accessed by different tasks, each executing in its own thread. What's the best way to give access to this? I want to keep everything as loosely coupled as possible. The shared global static variable in my ComputeEngine class will be a List. Should I have a getter for this static variable? I will have a read/write lock in my ComputeEngine class to give access to my global List. This too will be static and will need to be shared. I'm looking for best practice on how to provide access to a global static variable in a class.

Answer 1

If you want to decouple it, the best way is to pass a callback object when you create the Task.

interface FooListManipulator {
  void addFoo( Foo f );
  List<Foo> getFooList();
}

class Task {
  private FooListManipulator fooListManipulator;

  public Task( FooListManipulator fooListManipulator ) {
    this.fooListManipulator = fooListManipulator;
  }
}

This way the Task itself doesn't have to assume anything about who created it and how the list is stored.

And in your ComputeEngine you will do something like this:

class ComputeEngine {

  private static List<Foo> fooList;

  class Manipulator implements FooListManipulator {
    public void addFoo( Foo f ) {
      synchronized( fooList ) {
        fooList.add( f );
      }
    } 

    public List<Foo> getFooList() {
      return Collections.unmodifiableList( fooList );
    }
  }

  private Task createTask() {
    return new Task( new Manipulator() );  
  }
}

If you want to change the storage of fooList later (which you should really consider, as static global variables aren't a great idea), Task will remain unchanged. Plus you will be able to unit test Task with a mock manipulator.

Answer 2

I'm looking for best practice on how to provide access to a global static variable

By best practices, you shouldn't have such variables.

Should I have a getter for this static variable? I will have a read/write lock in my ComputeEngine class to give access to my global List.

No, you shouldn't provide such getter. Just addTask(Task task) or execute(task) method. Method synchronization will be workable solution.

Answer 3

Nooooooooooooooooooooooooooooooooooooooooooooooooooo!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Static mutables are Bad, and dressing it up as Singletons just make matters worse. Pass objects through constructors as necessary. And give objects sensible behaviour.

In the case of RMI, by default you are loading untrusted code from wherever directed by the client (top tip, when using RMI, use -Djava.rmi.server.useCodebaseOnly=true). As a global static, this code can fiddle with your server state (assuming in an accessible class loader, etc).

Answer 4

• don't return your list from the getter, as you won't know what people will do with it (they may add things and break your locking). So do this:

  static synchronized List getTheList() { return new ArrayList(theList); }

and only implement the getter if anyone actually needs it

• don't implement any setters; instead implement addItemToList() and removeItemToList()

other than that, having a global static variable is frowned upon...

Answer 5

I have several recommendations for you:

1. Your "Task" sounds like a Runnable, change "execute" to "run" and you get a lot of stuff for free. Like all the awesome classes in java.util.concurrent.
2. Make ComputeEngine itself a Singleton via the technique in this post. To be clear, use the "enum" approach from Josh Bloch (2nd answer on that question).
3. Make your List a member of ComputeEngine
4. Tasks use ComputeEngine.saveResult(...), which modifies the List.
5. Consider using java.util.concurrent.Executors to manage your pool of Tasks.

Answer 6

Going after @biziclop answer but with other separation)

You can separate your code in next parts.

interface Task {
    void execute();
}

public final class TaskExecutor{
     TaskExecutor(List<Task> tasks){}
     void addTask(Task task){synchronized(tasks){tasks.add(task);}}
}

Than,

public class SomeTaskAdder {
     SomeTaskAdder(TaskExecutor executor){}
     void foo(){
           executor.addTask(new GoodTask(bla-bla));
     }
}

public class SomeTasksUser {
     SomeTasksUser(List<Task> tasks){synchronized(tasks){bla-bla}}
}

than, you should create your objects with some magic constructor injection)

Answer 7

Everyone seems to be asserting you're using the List to keep a queue of Tasks, but I don't actually see that in your question. But if it is, or if the manipulations to the list are otherwise independent — that is, if you are simply adding to or removing from the list as opposed to say, scanning the list and removing some items from the middle as a part of the job — then you should consider using a BlockingQueue or QueueDeque instead of a List and simply leveraging the java.util.concurrent package. These types do not require external lock management.

If you require in earnest a List which is concurrently accessed by the each job, where the reads and writes to the list not independent, I would encapsulate the part of the processing which does this manipulation in a singleton, and use an exclusive lock to have each thread make use of the list. For instance, if your list contains some sort of aggregate statistics which are only a part of the process execution, then I would have my job be one class and a singleton aggregate statistics be a separate job.

class AggregateStatistics {
    private static final AggregateStatistics aggregateStatistics = 
                   new AggregateStatistics();

    public static AggregateStatistics getAggregateStatistics () {
           return aggregateStatistics;
    }

    private List list = new ArrayList ();
    private Lock lock = new ReentrantLock();

    public void updateAggregates (...) {
        lock.lock();
        try {
            /* Mutation of the list */
        }
        finally {
            lock.unlock();
        }
    }
}

Then have your task enter this portion of the job by accessing the singleton and calling the method on it which is managed with a lock.

Never pass a collection which into a concurrent environment, it will only cause you problems. You can always pass around an immutable "wrapper" though if it's really suitable, by using java.util.Collections.unmodifiableList(List) and similar methods.