Java selective synchronization

Question

I'm maintaining a very old application, and recently I came across a 'multi thread' bug. Here, in a method, to insert a value into a db, the record is first checked that it exists or not, then if it does not exist, it is inserted into the db.

createSomething(params)
{
  ....
  ....
  if( !isPresentInDb(params) )
  {
    .....
    .....
    .....
    insertIntoDb(params)
   }
 ...
 }

Here when multiple threads invoke this method, two or more threads with same params may cross the isPresentInDb check, one thread inserts successfully, the other threads fail.

To solve this problem I enclosed both the db interactions into a single synchronized(this) block. But is there a better way of doing this?

Edit: it is more like selective synchronization, only threads with same params need to by synchronized. Is selective synchronization possible?

Answer 1

I'd say the better way to do this would be to let the database do it for you if at all possible. Assuming the row on the database that you are wanting to either update or insert has a unique constraint on it, then my usual approach would be

• unconditionally insert the row
• if an SQLException occurs, check to see if it is due to a duplicate key on insert error, if it is, do the update, otherwise rethrow the SQLException.

If you can wrap those statements in a database transaction, then you don't have to worry about two threads trampling on each other.

Answer 2

If the logic is really "create this if it doesn't already exist", it could be better still to push the logic down into the database. For example, MySQL has "INSERT IGNORE" syntax that will cause it to ignore the insert if it would violate a primary key constraint. It may not be possible for your code, but worth considering.

Answer 3

This way of doing would only work if this object instance is the only one which inserts something in the table. If it's not, then two threads will synchronize on two different objects, and the synchronization won't work. To make it short : the object should be a singleton, and no other object should insert into this table.

Even if there is a unique object instance inserting, if you have any other application, or any other JVM, inserting in this table, then the synchronization won't bring you any guarantee.

Doing this is better than nothing, but doesn't guarantee that the insert will always succeed. If it doesn't, then the transaction will rollback due (hopefully) to a constraint violation. If you don't have any unique constraint to guarantee the uniqueness in the database, and you have several applications inserting in parallel, then you can't do anything to avoid duplicates.

Answer 4

Since you only want to forbid this method from running with the same params, you can use a ConcurrentMap instead and then call putIfAbsent and check its return value before proceeding. This will allow you to run the method concurrently for different arguments.

Answer 5

Looks fine to me. You can use some of the java.util.concurrent aids, like a ReentrantLock.

It will be better to utilize some sort of optimistic transactions: try to insert, and catch an exception. If the records has just been inserted, simply do nothing.

Answer 6

In one word NO. there is not better way than this. Since to make check-then-update kind of operations atomic you must have to put the logic inside a synchronized block.

Answer 7

You could make the whole method synchronized. I tend to find that a good marker for "this method only gets run by one thread at a time". That's my personal preference though.

Answer 8

The downside of too coarse-grained locking is performance degradation. If the method is called often, it will become a performance bottleneck. Here are two other approaches:

• Move your concurrent code into your database statement, if possible.
• Use a non-blocking data structure such as ConcurrentMap and maintain a list of known entries (must be warmed up on startup). This allows you two run the method with minimal locking, and without synchronizing the code. An atomic putIfAbsent() can be used to check if it must be added or not.

Answer 9

As others have stated your current approach is fine. Although depending on your requirements there are other things to consider

• Is this the only place in your application where you insert these records into the db? If no then the insert could still fail even with synchronisation
• How often does theoperation fail? If the number of times the operations fail compared to the number of times you run the method it may be beneficial to detect the failure by catching an appropriate exception. This may be beneficial due to the overhead involved in synchronising threads.
• What does your application need to do when it detects this kind of failure?

Answer 10

On first sight your solution seems ok, but if you want to change it here are two options:

• use db transactions
• use locks from java.util.concurrent.locks

   

    Lock lock = new ReentrantLock();
    .....

    createSomething(params)
    {
      ....
      ....
      try {
        lock.lock();
        if( !isPresentInDb(params) )
        {
          .....
          .....
          .....
          insertIntoDb(params)
        }
      finally {
        lock.unlock;
      }
    }