Java threads locking on a specific object

Question

I have a web application and I am using Oracle database and I have a method basically like this:

public static void saveSomethingImportantToDataBase(Object theObjectIwantToSave) {
      if (!methodThatChecksThatObjectAlreadyExists) {
         storemyObject() //pseudo code
     }
     // Have to do a lot other saving stuff, because it either saves everything or nothing
     commit() // pseudo code to actually commit all my changes to the database.
}

Right now there is no synchronization of any kind so n threads can of course access this method freely, the problem arises when 2 threads enter this method both check and of course there is nothing just yet, and then they can both commit the transaction, creating a duplicate object.

I do not want to solve this with a unique key identifier in my Database, because I don't think I should be catching that SQLException.

I also cannot check right before the commit, because there are several checks not only 1, which would take a considerable amount of time.

My experience with locks and threads is limited, but my idea is basically to lock this code on the object that it is receiving. I don't know if for example say I receive an Integer Object, and I lock on my Integer with value 1, would that only prevent threads with another Integer with value 1 from entering, and all the other threads with value != 1 can enter freely?, is this how it works?.

Also if this is how it works, how is the lock object compared? how is it determined that they are in fact the same object?. A good article on this would also be appreciated.

How would you solve this?.

Answer 1

Your idea is a good one. This is the simplistic/naive version, but it's unlikely to work:

public static void saveSomethingImportantToDataBase(Object theObjectIwantToSave) {
    synchronized (theObjectIwantToSave) {
        if (!methodThatChecksThatObjectAlreadyExists) {
            storemyObject() //pseudo code
        }
        // Have to do a lot other saving stuff, because it either saves everything or nothing
        commit() // pseudo code to actually commit all my changes to the database.
    }
}

This code uses the object itself as the lock. But it has to be the same object (ie objectInThreadA == objectInThreadB) if it's to work. If two threads are operating on an object that is a copy of each other - ie has the same "id" for example, then you'll need to either synchronize the whole method:

    public static synchronized void saveSomethingImportantToDataBase(Object theObjectIwantToSave) ...

which will of course greatly reduce concurrency (throughput will drop to one thread at a time using the method - to be avoided).

Or find a way to get the same lock object based on the save object, like this approach:

private static final ConcurrentHashMap<Object, Object> LOCKS = new ConcurrentHashMap<Object, Object>();
public static void saveSomethingImportantToDataBase(Object theObjectIwantToSave) {
    synchronized (LOCKS.putIfAbsent(theObjectIwantToSave.getId(), new Object())) {
        ....    
    }
    LOCKS.remove(theObjectIwantToSave.getId()); // Clean up lock object to stop memory leak
}

This last version it the recommended one: It will ensure that two save objects that share the same "id" are locked with the same lock object - the method ConcurrentHashMap.putIfAbsent() is threadsafe, so "this will work", and it requires only that objectInThreadA.getId().equals(objectInThreadB.getId()) to work properly. Also, the datatype of getId() can be anything, including primitives (eg int) due to java's autoboxing.

If you override equals() and hashcode() for your object, then you could use the object itself instead of object.getId(), and that would be an improvement (Thanks @TheCapn for pointing this out)

This solution will only work with in one JVM. If your servers are clustered, that a whole different ball game and java's locking mechanism will not help you. You'll have to use a clustered locking solution, which is beyond the scope of this answer.

Answer 2

Here is an option adapted from And360's comment on Bohemian's answer, that tries to avoid race conditions, etc. Though I prefer my other answer to this question over this one, slightly:

import java.util.HashMap;
import java.util.concurrent.atomic.AtomicInteger;

// it is no advantage of using ConcurrentHashMap, since we synchronize access to it
// (we need to in order to "get" the lock and increment/decrement it safely)
// AtomicInteger is just a mutable int value holder
// we don't actually need it to be atomic
static final HashMap<Object, AtomicInteger> locks = new HashMap<Integer, AtomicInteger>();

public static void saveSomethingImportantToDataBase(Object objectToSave) {
    AtomicInteger lock;
    synchronized (locks) {
        lock = locks.get(objectToSave.getId());
        if (lock == null) {
            lock = new AtomicInteger(1);
            locks.put(objectToSave.getId(), lock);
        }
        else 
          lock.incrementAndGet();
    }
    try {
        synchronized (lock) {
            // do synchronized work here (synchronized by objectToSave's id)
        }
    } finally {
        synchronized (locks) {
            lock.decrementAndGet();
            if (lock.get() == 0)  
              locks.remove(id);
        }
    }
}

You could split these out into helper methods "get lock object" and "release lock" or what not, as well, to cleanup the code. This way feels a little more kludgey than my other answer.