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 CommunicatorFactorys to choose whether to return a singleton Communicator or a new instance on demand, or something in-between. — Mena
– Mena, Commented Dec 11, 2017 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
– assylias, Commented Dec 11, 2017 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
– assylias, Commented Dec 11, 2017 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? — user7502825
– user7502825, Commented Dec 11, 2017 at 15:48
@dozer No I meant a static block: static { /* intialise the map */ } in your class. Not a static method. — assylias
– assylias, Commented Dec 11, 2017 at 16:04

score 1 · Accepted Answer · 2017-12-11 16:40:46Z

1

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.

edited Dec 11, 2017 at 16:40

answered Dec 11, 2017 at 16:00

user7502825

Sign up to request clarification or add additional context in comments.

2 Comments

dozer Over a year ago

Thanks. The last bit looks interesting but I get this error. The method put(String, Class<Communicator>) in the type Map<String,Class<Communicator>> is not applicable for the arguments (String, Class<ADCommunicator>) for each of the put calls. What am I doing wrong? Nevermind - I should use Map<String, Class<? extends Communicator>> based on risktop's answer. Thanks for the idea though.

user7502825 Over a year ago

@Dozer fixed. Sorry, I didnt have your classes to compile against.

riskop · Accepted Answer · 2017-12-11 16:37:49Z

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

Steve11235 · Accepted Answer · 2017-12-11 16:34:30Z

0

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.

edited Dec 11, 2017 at 16:34

user7502825

answered Dec 11, 2017 at 15:48

Steve11235

2,9432 gold badges20 silver badges20 bronze badges

Collectives™ on Stack Overflow

Impact of multiple threads using same object in a singleton class

3 Answers 3

Switch Over an enum (Java 5)

Switch over a String (Java 8)

A Map of Factory (factory of factories)

A Map of Class (reflection)

2 Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Switch Over an enum (Java 5)

Switch over a String (Java 8)

A Map of Factory (factory of factories)

A Map of Class (reflection)

2 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related