Impact of multiple threads using same object in a singleton class

Question

I'm designing a module which can support different datasources. My module gets the user's company id as inputs and I must call the appropriate class based on the company id. I'm trying to incorporate some good design and avoid conditional statements where possible.

I have a FetchDataSource singleton class with this method.

public class FetchDataSourceSingleton {

    private static Map<String, Communicator> communicatorMap;
    public static Communicator getCommunicatorInstance(String dataSourceType) {

        if (communicatorMap == null || communicatorMap.isEmpty())
            populateCommunicatorMap();

        if (communicatorMap.containsKey(dataSourceType))
            return communicatorMap.get(dataSourceType);
        return null; 
    }
.... other methods including populateCommunicatorMap()
}

"Communicator" is an interface, and the communicator map will return the appropriate instance. This is the populateCommunicatorMap() method in the same singleton class.

    private static void populateCommunicatorMap() {

        communicatorMap = new HashMap<String, Communicator>();
        communicatorMap.put("AD", new ADCommunicator());
        communicatorMap.put("DB2", new DB2Communicator());
        communicatorMap.put("MYSQL", new MYSQLCommunicator());
}

ADCommunicator, DB2Communicator and MYSQLCommunicator will implement the Communicator inteface.

The code seems to work in my test draft. The only concern I have is the HashMap will return the same object for all communication requests to the same type. I can't seem to avoid having the same instance in the hashmap if I want to avoid the conditional statements. Otherwise instead of the hashmap, I could have just make calls like this.

Communicator comm;
if (type = "AD") comm = new ADCommunicator();
if (type = "DB2") comm = new DB2Communicator();
if (type = "MYSQL") comm = new MYSQLCommunicator();

I've avoided this by using the hashmap to return an instance based on type. But then I can't avoid the singleton problem where I get the same instance.

In a multithreaded environment, which needs to support hundreds of thousands of communication requests at a time, this could be a problem considering I'll need to syncronize a lot of code in each of the Communicator classes. Is there a way I can avoid the syncronization and make it thread safe without impacting performance?

You could also have a factory of factories, where the values in your map are factories for your actual `Communicator` instances. It would be delegated to the `CommunicatorFactory`s to choose whether to return a singleton `Communicator` or a new instance on demand, or something in-between. — Mena, Dec 11 '17 at 15:41
Instead of populating the hashmap lazily, just populate it once and for all in a static block (and make sure it's never accessed after being initialised). Then you will be fine as long as the XxxCommunicator classes are thread safe. — assylias, Dec 11 '17 at 15:42
Side comment, `if (communicatorMap.containsKey(dataSourceType)) return communicatorMap.get(dataSourceType); return null;` is exactly the same as `return communicatorMap.get(dataSourceType);`. — assylias, Dec 11 '17 at 15:43
> "The only concern I have is the HashMap will return the same object for all communication requests to the same type." I'm confused, do you want single instances of your sources, or not? — , Dec 11 '17 at 15:48
@assylias thanks. the hashmap was a static variable - I've updated the code. I'm assuming this is what you meant. — dozer, Dec 11 '17 at 15:54
@user211221 - I'd rather have multiple instances of the various Communicator subclasses. But I couldn't find a way to do so, because this is the only apporach I could think of to avoid conditional statements. Using the hashmap makes it convenient, especially since I can fetch this off some properties and the code should support plugging in other datasources (or, in some cases, the same datasource for multiple keys) with minimum coding. Having the class return an individual instance per call, minus conditionals would be ideal and avoid the issues of multiple threads accessing the same object. — dozer, Dec 11 '17 at 15:57
@dozer No I meant a static block: `static { /* intialise the map */ }` in your class. Not a static method. — assylias, Dec 11 '17 at 16:04
@dozer you're welcome. I hadn't read your code fully before suggesting, so my own suggestion would be something optional, on top of what assylias is suggesting, which makes a lot of sense: you definitely don't want to risk populating the map every method call like you do now. — Mena, Dec 11 '17 at 16:15
@Mena - I'm only populating the map once, in the first call - note that the hashmap is a static class variable and I also have this. if (communicatorMap == null || communicatorMap.isEmpty()) populateCommunicatorMap(); assylias suggested using a static block instead which works too. But I don't see too much of an advantage. — dozer, Dec 11 '17 at 16:18
@dozer yep that's me not paying attention again, yours is just a lazy population, vs the eager population proposed by assylias (which I would still back up). The point then is to have the `Communicator` classes thread-safe as assylias was mentioning, but it depends a lot on what they're supposed to do concretely. So if you need a new `Communicator` instance for every call, even within the same datasource type (a hint that your `Communicator` instances are probably not thread-safe), a factory of factories would be in order. Otherwise, I guess you should be fine as is. — Mena, Dec 11 '17 at 16:26
@assylias and others, thanks for the answers. I've been wondering, is this all overkill and am I better off just putting conditionals? Are the design options I mentioned okay or is there something I haven't considered yet? — dozer, Dec 15 '17 at 11:42

score 1 · Accepted Answer · 2017-12-11T16:40:46.727

I can't seem to avoid having the same instance in the hashmap

You can use a switch instead of a bunch of ifs.

Switch Over an enum (Java 5)

Change type to be an enum in Java 5+, then you can switch on it. I'd recommend enums in general for type safety.

// type is-a enum Communicator.TYPE
switch(type) {
    case AD: return new ADCommunicator();
    case DB2: return new DB2Communicator();
    case MYSQL: return new MYSQLCommunicator();
    default: return null;
}

Switch over a String (Java 8)

Java 8 can switch over Strings directly.

// type is-a String
switch(type) {
    case "AD": return new ADCommunicator();
    case "DB2": return new DB2Communicator();
    case "MYSQL": return new MYSQLCommunicator();
    default: return null;
}

Switching over an enum will be as fast as a map, if not faster. Switching on the string will be as fast as a Map.

A Map of Factory (factory of factories)

Or have a map of factories:

private final static Map<String, Factory<? extends Communicator>> map;
static {
    map.put("AD", ADCommunicatorFactory.getInstance());
    //...
    map.put(null, NullFactory<Communicator>.getInstance());
} // populated on class-load. Eliminates race from lazy init

// on get
return map.get(type).make();

A Map of Class (reflection)

Or use the reflection API to make instances, but then it would probably be better to just use conditionals.

// on init
Map<String, Class<? extends Communicator>> map = new HashMap<>();
map.put("AD", ADCommunicator.class);

// on get
try {
    return (Communicator) map.get(type).newInstance();
} catch(InstantiationException | IllegalAccessException | NullPointerException e) {
    return null;
}

P.S. This all sounds like premature optimization. I doubt that determining which Communicator to use is going to be a bottleneck in your system.

Thanks. The last bit looks interesting but I get this error. The method put(String, Class) in the type Map> is not applicable for the arguments (String, Class) for each of the put calls. What am I doing wrong? Nevermind - I should use Map> based on risktop's answer. Thanks for the idea though. — dozer, Dec 11 '17 at 16:38
@Dozer fixed. Sorry, I didnt have your classes to compile against. — , Dec 11 '17 at 16:43

score 1 · Answer 2 · answered Dec 11 '17 at 16:37

If all your communicators can be constructed with empty argument list constructor, then you can store the type (class) of the communicator in the map instead of an instance. Then you can look up the type (java.lang.Class) from your communicatorMap and instantiate a new instance with java.lang.Class.newInstance().

For example:

public interface Communicator {
    void communicate();
}

public class Communicator1 implements Communicator {
    public void communicate() {
        System.out.println("communicator1 communicates");
    }
}

public class Communicator2 implements Communicator {
    public void communicate() {
        System.out.println("communicator2 communicates");
    }
}

public class CommuniicatorTest {
    public static void main(String[] args) throws Exception {
        Map<String, Class<? extends Communicator>> communicators = new HashMap<String, Class<? extends Communicator>>();
        communicators.put("Comm1", Communicator1.class);
        communicators.put("Comm2", Communicator2.class);
        Communicator comm2 = communicators.get("Comm2").newInstance();
        comm2.communicate();
        System.out.println("comm2: " + comm2);
        Communicator anotherComm2 = communicators.get("Comm2").newInstance();
        anotherComm2.communicate();
        System.out.println("anotherComm2: " + anotherComm2);
    }
}

result:

communicator2 communicates
comm2: pack.Communicator2@6bc7c054
communicator2 communicates
anotherComm2: pack.Communicator2@232204a1

Awesome. I'll try this. Thanks! – dozer Dec 11 '17 at 16:43 — dozer, Dec 11 '17 at 16:43

score 0 · Answer 3 · edited Dec 11 '17 at 16:34

Assylias is correct about using a static initializer. It runs when your class loads, which guarantees that the map will be loaded before anything else happens to the class.

You didn't show the declaration of the map; I assume that it is static.

private final static Map<String, Communicator> communicatorMap;
static {
    communicatorMap = new HashMap<>();
    communicatorMap.put("AD", new ADCommunicator());
    communicatorMap.put("DB2", new DB2Communicator());
    communicatorMap.put("MYSQL", new MYSQLCommunicator());
}; // populated on class-load. Eliminates race from lazy init

The remaining issue is the Communicator implementation. All this assumes that it is thread-safe as well.

Impact of multiple threads using same object in a singleton class

3 Answers3

Switch Over an enum (Java 5)

Switch over a String (Java 8)

A Map of Factory (factory of factories)

A Map of Class (reflection)