Announcement Announcement Module
Collapse
No announcement yet.
Race condition when using a transaction proxy and synchronized Page Title Module
Move Remove Collapse
X
Conversation Detail Module
Collapse
  • Filter
  • Time
  • Show
Clear All
new posts

  • Race condition when using a transaction proxy and synchronized

    Folks,

    We have cases where we need persistence logic like:

    Code:
    public void saveStuff(Stuff o)
    {
    	if (!exists(o));
    	    saveIt(o);
    }
    We've wrapped it in a Spring TransactionProxyFactoryBean with PROPAGATION_REQUIRED, so that it performs the commit after the method finishes. We also want thread safety (being a check & do action) so:

    Code:
    public synchronized void saveStuff(Stuff o)
    {
    	if (!exists(o));
    	    saveIt(o);
    }
    But is this fully thread safe? The scenario goes something like:

    Two threads with trying to save two identical Stuff objects simultaneously. Thread 1 calls saveStuff, the transaction proxy intercepts the call, starts a transaction and then acquires the lock. Thread 2 *also* attempts to call saveStuff, starts a transaction but then waits to acquire the lock. Thread 1 then finishes, exits the synchronized block. Thread 2qcquires lock, enters the exists() method, possibly *before* Spring's transaction management has had a chance to commit the transaction. exists() returns false and the save potentially occurs twice.

    Basically, the transaction exists outside the synchronized block, so a race condition may occur. Is this righht?

    The solution we had was pretty ugly.

    Code:
    public synchronized void saveStuff(Stuff o)
    {
            final DefaultTransactionDefinition transactionDefinition = new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
            final TransactionStatus transaction = transactionManager.getTransaction(transactionDefinition);
            try {
                if (!exists(o)) ;
                    saveIt(o);
                }
                transactionManager.commit(transaction);
            } catch (final RuntimeException exception) {
                transactionManager.rollback(transaction);
                throw exception;
            }
    }
    It seems to be the only way to ensure that the transaction code ran within the lock, but it's far from elegant.

    Is there a simpler way than this? Have we completely missed the point? What are some strategies people have used on this?

    Thanks,

    Mark C

  • #2
    Originally posted by wanderingwalrus View Post
    Basically, the transaction exists outside the synchronized block, so a race condition may occur. Is this righht?
    No, it can't from the code you posted. You do not even need to synchronize saveStuff(). That's only necessary if you have state in the class holding this method and if you access it in that method. This seems not to be the case. It's the database that has to lock the rows/tables appropriately. Don't mix transaction and Java synchronization.

    Joerg

    Comment


    • #3
      Joerg,

      Thanks for the reply.

      But if there is no locking and the "exists" method looks up the database, there's no guarantee that Thread 1 will have committed the save before Thread 2 calls "exists"? Without syncing wouldn't you have the situation:

      # Thread 1 calls exists returns false
      # Thread 2 calls exists returns false
      # Thread 1 calls save, adding a new row
      # Thread 2 calls save, adding another new row, which we don't want to happen

      with synchronization, I'd still imagine something like this is possible

      # Thread 1 calls exists returns false
      # Thread 1 calls save, adding a new row, but has not committed
      # Thread 2 calls exists returns false, since exists will check the database, but thread 1 hasn't committed
      # Thread 1 ends the transaction and commits
      # Thread 2 calls save, adding another new row, which we don't want to happen

      Originally posted by Jörg Heinicke View Post
      It's the database that has to lock the rows/tables appropriately.

      Would the dB level locking be able to cater for the case above? Wouldn't the locking only occur once for the read (exists), and then again for the write (save)?

      Thanks for the help!

      Cheers,

      Mark C

      Comment


      • #4
        Originally posted by wanderingwalrus View Post
        But if there is no locking and the "exists" method looks up the database, there's no guarantee that Thread 1 will have committed the save before Thread 2 calls "exists"?
        That's true. But there is workaround available with "select for update". Never have used it though since I don't have such exists checks. Anyway, this "select for update" would lock the table appropriately.

        Originally posted by wanderingwalrus View Post
        Wouldn't the locking only occur once for the read (exists), and then again for the write (save)?
        No, that must not happen as soon as you have the lock. The lock must not be released until the transaction is committed or rolled back. That's what transaction isolation is about.

        Another thread about business object concurrency might be of interest for you.

        Jörg

        Comment


        • #5
          Hi Jörg,

          Originally posted by Jörg Heinicke View Post
          But there is workaround available with "select for update". Never have used it though since I don't have such exists checks. Anyway, this "select for update" would lock the table appropriately.
          I'm not sure about that... PostgreSQL, for example, only locks the selected rows, not the entire table. In the case where no rows are returned, when we want locking, no locking would occur.

          Originally posted by Jörg Heinicke View Post
          That's what transaction isolation is about.
          I initially thought that using sufficiently high transaction isolation might be the solution, but, as the PostgresSQL manual explains, even the strongest level of isolation ("Serializable") won't prevent two concurrent transactions from both detecting the non-existence of the row.

          Regards,
          Adrian

          Comment


          • #6
            I see your problem. That's probably a case where you need to switch back to Java synchronization. So back to your original issue.

            I don't think there is an issue with the synchronized method or block since there is still a difference between "transaction" in Java and the transaction on the database. Though both threads might have created a Java "transaction" they can't do the same on the database. When the one thread finished the synchronized mehod but has not committed the transaction yet and the second thread jumps in the database table should already be locked and the second thread can't execute the exists() until the transaction of thread one has been committed - and the object created.

            Does that make sense?

            Joerg

            Comment

            Working...
            X