CODEWITHSUNDEEP
javathread-safetylazy-initialization
golfmonke
golfmonke

Reputation: 61

Java Thread Safe Lazy Initialization

I have a web application. Running on tomcat and multiple threads serving the Servlet calls.

I have a User class, an Account class, and an 1AccountContext` class.

Accounts can have multiple Users.

Only 1 instance of AccountContext should be maintained in memory per Account.

When a user makes a log in call via the servlet: if the AccountContext exists, return that. Otherwise, initialize it.

Below is the code that I wrote to initialize the context. Does this code look like it will do what I want while being thread safe?

ACCOUNT_CONTEXT_MAP is a ConcurrentHashMap.

public static AccountContext getAccountContext(Account account) {
    AccountContext accountContext = ACCOUNT_CONTEXT_MAP.get(account);
    if(accountContext == null){
        synchronized(account){
            if(ACCOUNT_CONTEXT_MAP.get(account) == null)
                accountContext = new AccountContext(account);
                //Creating the AccountContext is expensive, 
                //i'd like it if it was only done once.
                ACCOUNT_CONTEXT_MAP.put(account,accountContext);        
            }else{
                accountContext = ACCOUNT_CONTEXT_MAP.get(account);
            }
        }
    }
    return accountContext;
}

Upvotes: 2

Views: 1504

Answers (3)

Evgeniy Dorofeev
Evgeniy Dorofeev

Reputation: 135992

I would use ConcurrentHashMap.putIfAbsent atomic method instead of synchronization which is specially designed for such situations. This is how it's used:

AccountContext accountContext = ACCOUNT_CONTEXT_MAP.get(account);
if (accountContext == null){
    accountContext = new AccountContext(account);
    AccountContext accountContextOld = ACCOUNT_CONTEXT_MAP.putIfAbsent(account, accountContext);        
    if (accountContextOld != null) {
         accountContext = accountContextOld;
    }
}
return accountContext;

Upvotes: 1

morgano
morgano

Reputation: 17422

IMHO it is not thread safe unless you guarantee that all threads have the same instance of account and there is no way to have two Account objects representing the same "account", consider this scenario: two threads have an Account object each, representing the same account, they both call getAccountContext(), first thread get suspended right after the line if(accountContext == null) but before starting the initialization, then second thread gets to this same lime, verify that accountContext is null and proceeds to create the AccountContext, then the first thread is given again CPU time, as this first thread has already "verified" that accountContext is null, it will proceed to create another instance.

Try by synchronizing using the map itselt (ACCOUNT_CONTEXT_MAP) rather than each Account object.

If you don't want to synchronize on the map because it would cause other thread to wait for an expensive AccountContext to be created, then try this:

  • Create a new class: AccountContextBuilder: an unexpensive-to-create class that builds an expensive AccountContext. This class will contain a builder method to create an AccountContext or return a previously created one.
  • Make your map to contain instances of AccountContextBuilder rather than AccountContext.
  • Synchronize on the map (anyway you need to have it synchronized), this time it wont penalize other threads, because you are going to create a "cheap" builder object.
  • Finally, the thread use this builder to get access to the AccountContext, this way other threads aren't penalized because of other AccountContexts.

Upvotes: 1

yshavit
yshavit

Reputation: 43391

As @morgano pointed out, this only works if account is the same instance for all equal accounts. In addition, the Map must be thread-safe -- which often means using a ConcurrentHashMap or similar. If your map isn't thread-safe, then the get on the first line isn't thread-safe -- lots of bad things can go wrong.

One thing you can do is to stripe your locks. Create an array of N objects (literally Object is fine). When you need a lock for the synchronized block, get it from account.hashCode() % locksArray.length, and then synchronize on that object. This means you'll be able to create many AccountContexts in parallel, as long as their Accounts have different hashCode() % N. On average, this should give you good performance; obviously it assumes that Account has a suitable hashCode() override.

private final Object[] locks = createLocks();

private static Object[] createLocks() {
    Object[] locks = new Object[20]; // or whatever
    for (int i = 0; i < locks.length; ++i) {
        locks[i] = new Object();
    }
}

if (accountContext == null) {
    Object lock = locks[account % locks.length];
    synchronized (lock) {
        ...
    }
}

Lastly, this is a minor thing, but in the synch block, where you have:

if(ACCOUNT_CONTEXT_MAP.get(account) == null) {
    ...

I would do:

accountContext = ACCOUNT_CONTEXT_MAP.get(account);
if (accountContext == null) {
    ...

Then you don't need the else block.

Upvotes: 0

Related Questions