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

Orin

Why don't you think you should be catching the SQL exception? Oracle has built in features developed for decades by very smart people, exercised daily by tens of thousands of installed users, that solve your problem. Their wheel works fine. You're going to hate yourself for this kind of in memory locking as soon as your app gets big enough it needs to be clustered. – Temptation

@Temptation The app is in fact in a tomcat cluster already, so yes I am worried about thread locking for that reason... I suppose catching the SQL exception would work, I just wanted to see other possibilities, the thing with catching that SQL exception is that I would have to check it is the right ORA number, does that change often if ever? – Orin

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.

Goodden

How does java determine that the object is in fact the same object? – Orin

@OscarMk - You mean how does Java know that the two threads are referencing the same Object? In that case its identity in memory is enough to know what object it is accessing. For a more general understanding of your issue look up the "Reader/Writer Problem", a popular concurrency issue. – Beg

@TheCapn Ok I will look into that problem, so it basically determines equality with object 1 == object2 ??, is it possible for it to do a object1.equals(object2) instead?, They will not actually be referencing to the same object in memory, but they have the same attributes. And i want to lock on objects with the same attributes not the same location in memory. – Orin

@Goodden That does seem like it would work, but would that be a good solution? – Orin

Yes! IMHO the last version is the best approach. – Goodden

@OscarMk as long as you override .equals() within the Object to do a comparison on the attributes you'll get what you want. – Beg

@Goodden Would this be a problem in tomcat clustered environment? – Orin

This solution has a memory lick, this table of locks is growing out of control. And it forces your ID to be objects (not "int" or "char", for example). – Springs

@edutesoy - Did you even read my code? See the line : LOCKS.remove(theObjectIwantToSave.getId()); // Clean up lock object to stop memory leak. That cleans up completely. There is no memory leak. There are only as many entries in LOCKS as there are threads concurrently in the synchronized block. You are also wrong about the id not being able to be int char etc - java's autoboxing allows this - I've edited the answer to include a mention of autoboxing plus a link. You may consider following that link and reading the content there. – Goodden

@OscarMik: No: This won't work in a clustered environment - see edited answer. – Goodden

@Goodden you are right, i didn't look slowly to the code, this is a good solution. – Springs

@Goodden Ok, i will keep looking into that, thank you for your help. – Orin

It is what i looked for. But it seems there is a synchronisation error on remove call. If one thread has finished saving and has removed lock and second thread has started work then third thread can work before second thread will have finished. – Promote

@and390 work on any given object is one thread at a time. Another thread wanting to do work will halt until the previous thread releases the lock. That said, I haven't seen a locking pattern like this before, and I did not test this code, so it might not actually work. If you find this doesn't properly synchronise access, let me know and I'll investigate – Goodden

@Goodden This test code shows error: ideone.com/zEDBv3 (if i am not wrong, it additionally needs to handle null returned by putIfAbsent) – Promote

@Goodden It can be solved with references count for remove call. For example: ideone.com/CYSvEh. – Promote

The problem here is that if there are 3 threads, the first can remove the lock object while the second is still in the critical block, allowing the third to also enter the critical block with a new lock object...I'll add some warning to the answer at some point probably. – Auten

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.

Auten

Answer 3

Bohemian's answer seems to have race condition problems if one thread is in the synchronized section while another thread removes the synchro-object from the Map, etc. So here is an alternative that leverages WeakRef's.

// there is no synchronized weak hash map, apparently
// and Collections.synchronizedMap has no putIfAbsent method, so we use synchronized(locks) down below

WeakHashMap<Integer, Integer> locks = new WeakHashMap<>(); 

public void saveSomethingImportantToDataBase(DatabaseObject objectToSave) {
  Integer lock;
  synchronized (locks) {
    lock = locks.get(objectToSave.getId());
    if (lock == null) {
      lock = new Integer(objectToSave.getId());
      locks.put(lock, lock);
    }
  }
  synchronized (lock) {
    // synchronized work here (synchronized by objectToSave's id)
  }
  // no releasing needed, weakref does that for us, we're done!
}

And a more concrete example of how to use the above style system:

static WeakHashMap<Integer, Integer> locks = new WeakHashMap<>(); 

static Object getSyncObjectForId(int id) {
  synchronized (locks) {
    Integer lock = locks.get(id);
    if (lock == null) {
      lock = new Integer(id);
      locks.put(lock, lock);
    }
    return lock;
  }
}

Then use it elsewhere like this:

...
  synchronized (getSyncObjectForId(id)) {
    // synchronized work here
  }
...

The reason this works is basically that if two objects with matching keys enter the critical block, the second will retrieve the lock the first is already using (or the one that is left behind and hasn't been GC'ed yet). However if it is unused, both will have left the method behind and removed their references to the lock object, so it is safely collected.

If you have a limited "known size" of synchronization points you want to use (one that doesn't have to decrease in size eventually), you could probably avoid using a HashMap and use a ConcurrentHashMap instead, with its putIfAbsent method which might be easier to understand.

Auten

WeakRef is a good idea, but I think maybe your answer seems to have race condition problems, just like And390 comment. Simulation: Thread1 add a lock1, ref by his key1(new Integer(1234)). Thread2 try to getSyncObjectForId, key2(new Integer(1234)), get T1's lock1, and be locked. Thread1 leave, I think key2 !== key1, so there is no other reference to key1, since we use weakmap, this key1-lock1 entry may be removed from map when doing GC. Thread3 come, there is no lock in map, it enter the condition area while Thread2 is still running! – Paleolith

Brilliant solution. – Paleolith

Answer 4

My opinion is you are not struggling with a real threading problem.

You would be better off letting the DBMS automatically assign a non conflicting row id.

If you need to work with existing row ids store them as thread local variables. If there is no need for shared data do not share data between threads.

http://download.oracle.com/javase/6/docs/api/java/lang/ThreadLocal.html

An Oracle dbms is much better in keeping the data consistent when an application server or a web container.

"Many database systems automatically generate a unique key field when a row is inserted. Oracle Database provides the same functionality with the help of sequences and triggers. JDBC 3.0 introduces the retrieval of auto-generated keys feature that enables you to retrieve such generated values. In JDBC 3.0, the following interfaces are enhanced to support the retrieval of auto-generated keys feature ...."

http://download.oracle.com/docs/cd/B19306_01/java.102/b14355/jdbcvers.htm#CHDEGDHJ

Spandau

The problem is that nothing is actually inserted till the commit(). – Orin

I think you should be able to retrieve the keys also before you finally commit the transaction. Just assume pstmt is your prepared statemt and you have already executed it. ResultSet generatedKeys = pstmt.getGeneratedKeys(); – Spandau

Answer 5

If you can live with occasional over-synchronization (ie. work done sequentially when not needed) try this:

1. Create a table with lock objects. The bigger table, the fewer chances for over-synchronizaton.
2. Apply some hashing function to your id to compute table index. If your id is numeric, you can just use a remainder (modulo) function, if it is a String, use hashCode() and a remainder.
3. Get a lock from the table and synchronize on it.

An IdLock class:

public class IdLock {

private Object[] locks = new Object[10000];

public IdLock() {
  for (int i = 0; i < locks.length; i++) {
    locks[i] = new Object();
  }
}

public Object getLock(int id) {
  int index = id % locks.length;
  return locks[index];
}

}

and its use:

private idLock = new IdLock();

public void saveSomethingImportantToDataBase(Object theObjectIwantToSave) {
  synchronized (idLock.getLock(theObjectIwantToSave.getId())) {
    // synchronized work here
  }
}

Arrears

Answer 6

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

The synchronized keyword locks the object you want so that no other method could access it.

Franza

Answer 7

I don't think you have any choice but to take one of the solutions that you do not seem to want to do.

In your case, I don't think any type of synchronization on the objectYouWantToSave is going to work since they are based on web requests. Therefore each request (on its own thread) is most likely going to have it's own instance of the object. Even though they might be considered logically equal, that doesn't matter for synchronization.

Lamp

Answer 8

synchronized keyword (or another sync operation) is must but is not enough for your problem. You should use a data structure to store which integer values are used. In our example HashSet is used. Do not forget clean too old record from hashset.

private static HashSet <Integer>isUsed= new HashSet <Integer>();

public synchronized static void saveSomethingImportantToDataBase(Object theObjectIwantToSave) {

      if(isUsed.contains(theObjectIwantToSave.your_integer_value) != null) {

      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.
      isUsed.add(theObjectIwantToSave.your_integer_value);

  }
}

Friedafriedberg

Answer 9

To answer your question about locking the Integer, the short answer is NO - it won't prevent threads with another Integer instance with the same value from entering. The long answer: depends on how you obtain the Integer - by constructor, by reusing some instances or by valueOf (that uses some caching). Anyway, I wouldn't rely on it.

A working solution that will work is to make the method synchronized:

public static synchronized 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.
}

This is probably not the best solution performance-wise, but it is guaranteed to work (note, if you are not in a clustered environment) until you find a better solution.

Snug

Answer 10

private static final Set<Object> lockedObjects = new HashSet<>();

private void lockObject(Object dbObject) throws InterruptedException {
    synchronized (lockedObjects) {
        while (!lockedObjects.add(dbObject)) {
            lockedObjects.wait();
        }
    }
}

private void unlockObject(Object dbObject) {
    synchronized (lockedObjects) {
        lockedObjects.remove(dbObject);
        lockedObjects.notifyAll();
    }
}

public void saveSomethingImportantToDatabase(Object theObjectIwantToSave) throws InterruptedException {
    try {
        lockObject(theObjectIwantToSave);

        if (!methodThatChecksThatObjectAlreadyExists(theObjectIwantToSave)) {
            storeMyObject(theObjectIwantToSave);
        }
        commit();
    } finally {
        unlockObject(theObjectIwantToSave);
    }
}

• You must correctly override methods 'equals' and 'hashCode' for your objects' classes. If you have unique id (String or Number) inside your object then you can just check this id instead of the whole object and no need to override 'equals' and 'hashCode'.
• try-finally - is very important - you must guarantee to unlock waiting threads after your operation even if your operation threw exception.
• This approach will not work if your back-end is distributed across multiple servers.

Shows