Implementing a thread-safe factory with caching

Question

I have a ConnectorFactory that creates Connector objects based on parameters like URLs, username and passwords.

The Connector object itself implements a HTTP connection pool internally, and can handle multiple HTTP requests concurrently.

I would like to do is modify my factory to cache the Connector objects. So it will get an existing Connector object if one already exists for the particular URL/username/password combination, or a new instance otherwise.

The newConnection() method needs to be thread safe, but rather than declaring it synchronized I have opted for a Double-checked locking-style solution.

Just about every reference I found on double checked locking relates to the singleton pattern, and requires use of the volatile keyword. Since I'm not using my Factory in this way, I don't find those relevant.

Here are the relevant parts of the ConnectorFactory I have come up with. I would like for an expert to tell me if it is correct, and if not, what the correct way to do it would be.

private static Map<ConnectionSettings, Connector> connectorCache = new ConcurrentHashMap<ConnectionSettings, Connector>();
public static Connector newConnection(String url, String user, String psswd) throws ConfigurationException {
 ConnectionSettings settings = new ConnectionSettings(url, user, psswd);
 Connector connector = connectorCache.get(settings);
 if (connector == null) {
 synchronized (ConnectorFactory.class) {
 connector = connectorCache.get(settings);
 if (connector == null) {
 connector = new Connector(settings);
 connectorCache.put(settings, connector);
 }
 }
 }
 return connector;
}

I have overridden the equals() and hashCode() methods on ConnectionSettings.

Finally, if only this method accesses connectorCache, can I change it to a normal HashMap rather than a ConcurrentHashMap?

• fge: Why don't you use an already existing cache implementation, for instance Guava's LoadingCache?

Answer 1

Ouch. You have the right idea with the ConcurrentHashMap, but then do a system-wide synchronization on the ConnectorFactory.class?

Double-checked locking is effectively available using the functionality provided by the ConcurrentMap interface. Consider your code rewritten as (and using putIfAbsent())

private static ConcurrentMap<ConnectionSettings, Connector> connectorCache = new ConcurrentHashMap<ConnectionSettings, Connector>();
public static Connector newConnection(String url, String user, String psswd) throws ConfigurationException {
 ConnectionSettings settings = new ConnectionSettings(url, user, psswd);
 // do an atomic get, first-check.
 Connector connector = connectorCache.get(settings);
 if (connector == null) {
 // There's a good chance we're the first thread to request this settings
 // be optimistic, and assume we are the the only one.
 connector = new Connector(settings);
 // atomic second-check
 Connector race = connectorCache.putIfAbsent(settings, connector);
 if (race != null) {
 // some other thread managed to do the double-check at the same
 // time as us, and win the race to the putIfAbsent
 // use the winner's Connector, (and abandon ours).
 connector.abandon(); // if you need to dispose of stuff....
 // return the winner
 return race;
 }
 }
 return connector;
}

The above code uses an optimistic approach of first checking if the connector exists, creating it if it doesn't, then double-checking it. There is no global synchronization.

The downside is that occasionally you may create and discard duplicate connectors in the unlikely event that, at an exact moment in time when the connector is being referenced for the first time, that two or more threads are doing it concurrently. The cost is simply the cost of losing the race.

Answer 2

If you are on Java 8, you can use a cool new feature of ConcurrentMap: computeIfAbsent.

It lets you defer the actual construction of the Connection until it becomes necessary and makes your code more concise:

private static ConcurrentMap<ConnectionSettings, Connector> connectorCache =
 new ConcurrentHashMap<ConnectionSettings, Connector>();
public static Connector newConnection(String url, String user, String psswd)
 throws ConfigurationException {
 ConnectionSettings settings = new ConnectionSettings(url, user, psswd);
 return connectorCache.computeIfAbsent(settings, Connector::new);
}

Note the two arguments to computeIfAbsent:

1. The first is your ConnectionSettings instance, which is used as a key.
2. The second is an instance of the interface Function. If called, it gets the settings as an argument and must return an instance of Connector. The function is defined by using a lambda expression, in this case a constructor reference. This is possible because the constructor of Connector does exactly that: it takes the settings as an argument and returns a Connector instance.

The call tells your connectorCache to check for the specified settings. If they exist the corresponding connector is returned. If not (and only then!) is the function executed, which creates a new Connector. (Of course in that case the new instance is put into the map.)

So you see the constructor is only invoked if needed. All necessary locking for concurrency is done by the map and you don't have to worry about it.

Answer 3

I haven't seen the code for ConnectionSettings, but it appears that the following line is not thread-safe.

ConnectionSettings settings = new ConnectionSettings(url, user, psswd);

• Billal BEGUERADJ: Yes, it is unsafe, good spot, but it is interesting to mention how could the OP remedy to this.