6

I have a Singleton class to save the state of an application's module. This class simply have a lot of class variables with setters and getters :

public class ModuleState{

private static ModuleState instance;

private A a;
private B b;
private C c;
..
..
..
..

private ModuleState (){}

public ModuleState getInstance(){
    if(instance==null)
        instance=new ModuleState();

    return instance;
}

}

At a precise moment of the application lifecycle, i have the need to CLEAR the module's state. What i do now is to reset ALL the variables in ModuleState by a clearAll() method like this:

public void clearAll(){
    a=null;
    b=null;
    c=null;

    ..
    ..
}

My question is the following : there is a cleaner method to do this reset? Possibly clearing the singleton instance itself, without resetting every class variable?

The problem with this approach is that i may have the need to add a new class variable to the ModuleState. In this case i must remember to add a line in the clearAll() method to reset the new variable.

Nameless
  • 259
  • 2
  • 4
  • 15

5 Answers5

12

What about ...

public static volatile ModuleState instance = null;

public static void reset() {
    instance = new ModuleState();
}

p.s.: as per discussion below: in a multithreaded environment it's very important to synchronize the access on the instance because the JVM is allowed to cache its value. You can use volatile as shown above. Thanks to all!

Cheers!

Community
  • 1
  • 1
Trinimon
  • 13,839
  • 9
  • 44
  • 60
  • 9
    The singleton wouldn't be a singleton anymore. If some code caches the result of getInstance() somewhere, it would then access or modify the previous version of the state. That has to be taken into account before choosing this solution. – JB Nizet May 12 '13 at 09:42
  • Yes, you are right: it's a bit against the idea of a Singleton. In addition it might be necessary to synchronize this method. But still functional, isn't it? – Trinimon May 12 '13 at 09:45
  • 1
    It has to cache it somehow. Just calling ModuleState.getInstance().getA().getFoo() has a chance of accessing a stale state if another thread changes the singleton in parallel. Making getInstance() synchronized doesn't change anything. – JB Nizet May 12 '13 at 09:46
  • Could that be solved just by converting the entire class to an enum? Then, there only would be one instance type and it would be synchronised. – Andrew Martin May 12 '13 at 09:46
  • The instance variable should be declared final really, but that does prevent lazy instantiation. A reset method should clear all values within the instance, so anything using the instance are using the reset version. – eldris May 12 '13 at 09:47
  • IMHO, the only safe singleton is a stateless one. – Peter Lawrey May 12 '13 at 09:56
  • It's no common practice to declare the variable `final`. You can do this but it's not required (and it avoids lazy loading as you mentioned). Main feature of a Singleton is to have only one instance - that's still the case. Only issue to care about is access from different threads - however this can be solved by using `synchronized` or `volatile`. – Trinimon May 12 '13 at 09:59
  • 1
    @JBNizet The same unsafety argument can then apply to individual objects returned by `getA..Z`, so obviously some practical limit to "precision" must be accepted. Making `instance` volatile and using this approach is probably a very decent solution to OP's problem. – Marko Topolnik May 12 '13 at 09:59
  • @MarkoTopolnik agreed. I wasn't saying this solution was inherently bad and didn't downvote. I was just mentioning that it could have serious side-effects if the singleton was cached, which is often the case if the singleton is a real singleton. clearijg the state of the singleton at least allows for encapsulation and non-staleness. Replacing the singleton by another one doesn't allow that. – JB Nizet May 12 '13 at 10:15
  • 1
    @JBNizet The only ultimately safe solution is a massive class which never exposes any of `A..Z` objects, but can only be queried for the leaf atomic values they hold. The whole class would have to be `synchronized` so that the *reset* operation is mutually exclusive with each *get*. – Marko Topolnik May 12 '13 at 10:17
  • Yes indeed. Or A..Z should themselves be atomic values, i.e. immutable objects. – JB Nizet May 12 '13 at 10:20
  • @Marko Topolnik Can you give an example of the "only ultimately safe solution" ? thanks – Nameless May 12 '13 at 11:51
2

no, this approach is perfectly acceptable. you are of course synchronizing access to these state objects in some way, right? otherwise you risk someone seeing a half-cleared config object.

another thing you could do to future-proof yourself against any extra state added in the future is store all of your state in a HashMap, for example, instead of individual fields. this way, clear()ing the hashmap ensures that all state is wiped and adding any extra state in the future becomes safer

radai
  • 23,949
  • 10
  • 71
  • 115
1

You need to maintain the same object instance, in order to comply with the Singleton pattern, so your approach makes sense: altering the members.

However, if you wanted to clean it up a little bit, why not just have an internal list, like:

 ArrayList<Object> members = new ArrayList<Object>();
 // If it actually is Object, there's no need to paramaterize.
 // If you want, you can actually make the members implement a common interface,
 // and parameterize the ArrayList to that.

Another Option would be to have a HashMap, that binds the key word to the member.

 HashMap<String,Object> members = new HashMap<String,Object>();
 // Again, same parameterization rules apply.

For an ArrayList or a HashMap, the clearAll method might look like this:

public class ModuleState()
{
    public void clearAll()
    {
          members.clear();
    }
}

This method won't need to change.

christopher
  • 26,815
  • 5
  • 55
  • 89
0

Make an inner class to hold the fields, then replace that instance when you want to reset. The write to the field would make the change to all three fields essentially atomic.

public class ModuleState {

private static volatile ModuleState instance;

private static class Values {
    A a;
    B b;
    C c;
}
private volatile Values values = new Values()(

private ModuleState (){}

public ModuleState getInstance(){
    if (instance==null) {
        synchronized (ModuleState.class) {
            if (instance==null) {
                instance = new ModuleState();
            }
        }
    }

    return instance;
}

public synchronized A getA() {
     return values.a;
}

public synchronized void reset() {
    values = new Values();
}

By the way, your null checking initialization code was not threadsafe. I fixed that too.

Note that to make this work, you must make the reference to values volatile and synchronize all access to it, otherwise (due to the java memory model) other threads than the one that calls reset() may see the old reference.

Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • it seems like the singleton pattern is perhaps not the best fit here. if you have an object who's state you need to completely reset that sounds like the kind of thing you have constructors for. I would need to see a solid reason for introducing the above code which IMO is overly complicated. – Woody May 12 '13 at 10:25
  • @Woody Actually it doesn't achieve anything more than the simpler approach, it just allows the client to cache the "singleton". But there is no intrinsic value in having that kind of protocol: what's wrong with insisting that the singleton be always acquired from `getInstance`? – Marko Topolnik May 12 '13 at 11:03
  • @MarkoTopolnik Did you mean this when you talked about "a massive class which never exposes any of A..Z objects, but can only be queried for the leaf atomic values they hold. The whole class would have to be synchronized so that the reset operation is mutually exclusive with each get" ? By the way i like this solution, any downside? Is this safe? – Nameless May 12 '13 at 12:26
0

May be this can help you:

public class SingletonBean {

    private static SingletonBean instance = new SingletonBean();
    private static Object privateMutex = new Object();

    private SingletonBean() {
        //to prevent instantiation
    }



public class ObjectsContainer {
    private Object A;
    private Object B;
    private Object C;

    public Object getA() {
        return A;
    }

    public void setA(Object a) {
        A = a;
    }

    public Object getB() {
        return B;
    }

    public void setB(Object b) {
        B = b;
    }

    public Object getC() {
        return C;
    }

    public void setC(Object c) {
        C = c;
    }
}

    private ObjectsContainer objectsContainer;

    private void resetObjectsContainer() {
        objectsContainer = new ObjectsContainer();
    }

    public static SingletonBean getInstance() {
        return SingletonBean.instance;
    }

    public static void clearAll() {
        synchronized (privateMutex) {
            SingletonBean.getInstance().resetObjectsContainer();
        }
    }

    public static ObjectsContainer getObjectsContainer() {
        synchronized (privateMutex) {
            return instance.objectsContainer;
        }
    }
}

public class SomeClass {
    public void someMethod() {
        SingletonBean.getObjectsContainer().getA();
    }
}
gluckonavt
  • 236
  • 1
  • 4