Spring @Transactional with synchronized keyword doesn't work

Question

Let's say I have a java class with a method like this (just an example)

@Transactional
public synchronized void onRequest(Request request) {

    if (request.shouldAddBook()) {
        if (database.getByName(request.getBook().getName()) == null) {
            database.add(request.getBook());
        } else {
            throw new Exception("Cannot add book - book already exist");
        }
    } else if (request.shouldRemoveBook()) {
        if (database.getByName(request.getBook().getName()) != null) {
            removeBook();
        } else {
            throw new Exception("Cannot remove book - book doesn't exist");
        }
    }
}

Say this book gets removed and then re-added with a new author or other minor change, so this method might get called twice very fast from another system, first to remove the Book, then to add the same Book back (with some new details).

To handle this we might try (like I did) to add the above @Transactional code, and then also 'synchronized' when the @Transactional doesn't work. But strangely it fails on the second call with

"Cannot add book - book already exist".

I spent a lot of time trying to figure this out, so thought I'd share the answer.

Lamaism

If you want to change the details, you update it. You don't remove and add it. This is horrible design. If you do things properly, you won't need synchronized. – Youlandayoulton

This issue was not really found because I wanted to update details of a book. That would be bad design like you say. It is just an example. There are however other reasons why this pattern might occur, and someone else may hit this strange situation just like I did, so I wanted to show others why it doesn't work so they don't have to work so long with trying to understand it as I did. – Lamaism

same issue for me.for example create objects with checking duplicate names. i solve this problem with syncronize caller method of onRequest. – Incorrect

Answer 1

When removing and immediately adding back a Book, if we have no "@Transactional" or "synchronized" we are starting with this thread execution:

T1: |-----remove book----->

T2: |-------add book------->

The synchronized keyword makes sure that the method can only be run by one thread at a time. This means execution becomes this:

T1: |-----remove book-----> T2: |--------add book------>

The @Transactional annotation is an Aspect, and what it does is that it creates a proxy java class around your class, adds some code (begin transaction) to it before the method call, calls the method and then calls some other code (commit transaction). So the first thread now looks like this:

T1: |--Spring begins transaction--|-----remove book----- |--Spring commits transaction --->

or shorter: T1: |-B-|-R-|-C-->

and the second thread like this:

T2: |--Spring begins transaction--|-------add book------- |--Spring commits transaction --->

T2: |-B-|-A-|-C-->

Notice that the @Transactional annotation only locks the same entity in a database from being modified concurrently. Since we are adding a different entity (but with the same book name), it doesn't do much good. But it still shouldn't HURT right?

WELL HERE IS THE FUN PART:

The transaction code that Spring adds is not part of the synchronized method, so the T2 thread can actually start its method before the "commit" code is finished running, right after the first method call is done. Like this:

T1: |-B-|-R-|-C--|-->

T2: |-B------|-A-|-C-->

So. when the "add" method reads the database, the remove code has been run, but NOT the commit code, so it still finds the object in the database and throws the error. milliseconds later it will be gone from the database though.

Removing the @Transactional annotation would make the synchronized keyword work as expected, although this is not a good solution as others have mentioned. Removing synchronized and fixing the @Transactional annotation is a better solution.

Lamaism

Of course the real problem here is the use of synchronized. That's not something you want to put into data access code. This is essentially a bad fix for broken design. – Youlandayoulton

If he removes the @Transactional annotation how will he manage the Transaction ? That's not the way to solve the problem even if I agree that synchronized is the problem. – Scottyscotus

agree it may not be "solving the problem" more like "removing the specific situation you're in". – Lamaism

@Lamaism Would adding @Transactional(isolation = Isolation.READ_UNCOMMITTED) solve the issue here? I am facing a similar situation – Westbound

@NickDiv no I don't think so, see my comment on the other answer. I tried all isolation levels and none solved the issue at hand. – Lamaism

@Lamaism So how were you able to solve it. I cannot remove the Transactional :( – Westbound

@NickDiv it depends on what you want to limit in your case. If you have issues with multiple threads calling the same java method, then you can use syncronized keyword, but it's very rare that this is needed. If you have issues with ONE database object being read/written by different threads then the transactional keyword will be possible to use. But for this specific case where two different object are handled (they just happen to have the same name) the transactional won't work for you. – Lamaism

@NickDiv so in this case I don't remember what we did eventually, but try setting the name to unique in the database and just trying to add it without first reading the object as I do in my example code. Do this with transactional annotation. Maybe it can work for you? – Lamaism

@Lamaism Your explanation was the first that came to mind for me as well. BUT I thought that in this case if I create a private final static Object lock = new Object() and syncrhonize on it before I call the method instead of a synchronized method. Then the whole transaction would be inside the lock and it should work as expected. It does not work like that. I wonder why this is – Mcglothlin

I have similar use case where I am having synchronized method and transactional annotation. I am trying to read some data which has been updated as part of same method, but I am not getting it. Should I start reading uncommitted data? I can't move synchronized keyword to calling method. – Dett

Answer 2

You need to set your transaction isolation level to protect from dirty reads from the database, not worry about thread safety.

@Transactional(isolation = Isolation.SERIALIZABLE)
public void onRequest(Request request) {

    if (request.shouldAddBook()) {
        if (database.getByName(request.getBook().getName()) == null) {
            database.add(request.getBook());
        } else {
            throw new Exception("Cannot add book - book already exist");
        }
    } else if (request.shouldRemoveBook()) {
        if (database.getByName(request.getBook().getName()) != null) {
            removeBook();
        } else {
            throw new Exception("Cannot remove book - book doesn't exist");
        }
    }
}

Here is an excellent explanation of Transaction Propagation and Isolation.

Spring @Transactional - isolation, propagation

Endemic

Wouldn't the database consider the two book objects to be different and allow reading and writing them concurrently? – Lamaism

Database don't do it by default, but I think it can be configured in database level. This is a DBA stuff and I am not expert on that, So, I consider to make it at code level :) – Endemic

After trying all isolation levels I can say that (atleast in my example on my current setup) both threads are always allowed to read the value from the database even though another thread has read the same value recently without committing it's transaction. The only thing that works so far is the synchronized keyword... – Lamaism

Using SERIALIZABLE prevents dirty reads but you might end up with an exception if two or more threads are accessing the method simultaneously. Is there any way to prevent this? ERROR: could not serialize access due to read/write dependencies among transactions Detail: Reason code: Canceled on identification as a pivot, during write. Hint: The transaction might succeed if retried. – Andreas

@Endemic Would adding @Transactional(isolation = Isolation.READ_UNCOMMITTED) solve the issue here? I am facing a similar situation – Westbound

I am facing some issues using this solution, I´ve mentioned it here: #76254173 – Griffon

The SERIALIZABLE isolation level can have a performance impact, as it requires the database to maintain a higher level of locking. In some cases, a lower isolation level such as REPEATABLE_READ may be sufficient, depending on the specific requirements of the application. – Upswell

Answer 3

the 'synchronized' should used before the @Transaction method. Otherwise, when multithreading, the object has unlocked, but the transaction is not submitted.

Demello

Answer 4

I add 'synchronized' to the caller method of onRequest and change isolation of Transactional to READ_UNCOMMITTED. This can solve the issue. But I am curious why only moving 'synchronized' to the caller method doesn't work.Becaues by doing that, method executing process like this: hold synchronized lock, hold transaciton lock, release transaction lock, release synchronized lock. However the fourth step seems happen before the third step. So modifying transaction isolation is necessary. Anyone knows the reason?

Calaverite