Implementing a thread-safe factory with caching

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 connectorCache = new ConcurrentHashMap();

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?

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 connectorCache =
    new ConcurrentHashMap();

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:

• The first is your ConnectionSettings instance, which is used as a key.

• 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.

StackExchange Code Review Q#68156, answer score: 6