ycomp
ycomp

Reputation: 8583

Synchronized (Hash)Map of Singletons

Description below the code...

// Singleton
public static final Map<String, Account> SHARED_ACCOUNT_HASHMAP =
        Collections.synchronizedMap(new HashMap<>());


public init(String[] credentials) {

    Account account = null;     

    String uniqueID = uniqueAccountIdentifier(credentials);

    if (SHARED_ACCOUNT_HASHMAP.containsKey(uniqueID)) {
        account = SHARED_ACCOUNT_HASHMAP.get(uniqueID);
        log("...retrieved Shared Account object: %s", uniqueID);
    }

    // create the Account object (if necessary)
    if (account == null) {
        account = new Account(credentials);

        // Store it in the SHARED_ACCOUNT_HASHMAP
        SHARED_ACCOUNT_HASHMAP.put(uniqueID, account);
        log("...created Account object: %s",uniqueID);

    }

}

What I want to achieve

My problem...

Upvotes: 0

Views: 709

Answers (3)

akhil_mittal
akhil_mittal

Reputation: 24167

In the following code I can feel smell of race condition check-then-act as you are trying to perform two operations on the synchronised map (containsKey and get):

if (SHARED_ACCOUNT_HASHMAP.containsKey(uniqueID)) {
        account = SHARED_ACCOUNT_HASHMAP.get(uniqueID);
        log("...retrieved Shared Account object: %s", uniqueID);
 }

So to avoid race condition you need to synchronize over this map as:

synchronized (synchronizedMap) {
  if (SHARED_ACCOUNT_HASHMAP.containsKey(uniqueID)) {
            account = SHARED_ACCOUNT_HASHMAP.get(uniqueID);
            log("...retrieved Shared Account object: %s", uniqueID);
     }
  // rest of the code.      
}

Actually the synchronizedMap can protect itself against internal race conditions that could corrupt the map data but for external conditions (like above) you need to take care of that. If you feel you are using synchronized block at many places you can also think of using a regular map along with synchronized blocks. You will find this question also useful.

Upvotes: 1

codejitsu
codejitsu

Reputation: 3182

Consider using ReadWriteLock if you have multiple readers/writers (see ReadWriteLock example).

Generally the ConcurrentHashMap performs better than the sinchronized hash map you are using.

Upvotes: 1

epoch
epoch

Reputation: 16615

According to the docs for synchronizedMap()

Returns a synchronized (thread-safe) map backed by the specified map. In order to guarantee serial access, it is critical that all access to the backing map is accomplished through the returned map.

It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views

In other words you still need to have synchronized access to SHARED_ACCOUNT_HASHMAP:

public init(String[] credentials) {
    Account account = null;     
    String uniqueID = uniqueAccountIdentifier(credentials);

    synchronized (SHARED_ACCOUNT_HASHMAP) {
        if (SHARED_ACCOUNT_HASHMAP.containsKey(uniqueID)) {
            account = SHARED_ACCOUNT_HASHMAP.get(uniqueID);
            log("...retrieved Shared Account object: %s", uniqueID);
        }

        // create the Account object (if necessary)
        if (account == null) {
            account = new Account(credentials);

            // Store it in the SHARED_ACCOUNT_HASHMAP
            SHARED_ACCOUNT_HASHMAP.put(uniqueID, account);
            log("...created Account object: %s",uniqueID);
        }
    }
}

Upvotes: 1

Related Questions