Prevent corrupted ThreadLocals when mis-using triggerAfterCommit [SPR-11761]

[spring-projects-issues]

Aldrin Seychell opened SPR-11761 and commented

This issue started when we noticed a number of missing data in our database and not matching our reports, all resulting from a particular thread. This problem was being triggered after utilizing the TransactionSynchronizationManager.registerSynchronization method and implementing the afterCompletion method to trigger a separate transaction to perform some further logic afterwards. However, when calling a method annotated with a @Transactional(propagation=NEVER), this seems to be corrupting the threadLocal variables and the transactional resources being tied to thread indefinitely. Any subsequent calls from this thread to persist new data will never reach the database but will never fail.

---

Affects: 3.1.1, 4.0.4

Reference URL: spring-attic/spring-framework-issues#74

Attachments:

• 0001-SPR-11761.patch (2.60 kB)

Issue Links:

• #16214 TransactionSynchronizationManager - throw an Exception or log a warning if a Synchronization wants to add a Synchronization and afterCompletion is already called ("duplicates")
• #18362 When using Programatic transaction management of SPR if the developer misses to commit or rollback the transaction ; when the thread is returned from pool next time the stale transaction is returned.

Referenced from: commits spring-attic/spring-framework-issues@8945dd9, spring-attic/spring-framework-issues@41578e8

1 votes, 4 watchers

[spring-projects-issues]

Stéphane Nicoll commented

See repro project: https://github.com/spring-projects/spring-framework-issues/tree/master/SPR-11761

[spring-projects-issues]

Stéphane Nicoll commented

Removing the @Transactional(propagation = Propagation.NEVER) and the error goes away indeed. I am confused as why you use NEVER in the first place. Can you explain?

I haven't got what is going on but in the after completion hook you make sure that you can't use a transaction and then right after you invoke a method with requires new. I am a bit confused as why you're doing that.

Any subsequent calls from this thread to persist new data will never reach the database but will never fail.

That's not what the test shows as Book 4 (D) is found and is processed by a transaction that is created after the problematic call. Or did I misunderstood what you meant?

[spring-projects-issues]

Aldrin Seychell commented

Attaching patch file with a our "proposed" fix for this bug.

[spring-projects-issues]

Aldrin Seychell commented

The method is annotated with @Transactional(propagation = Propagation.NEVER) since it is used from other parts of the code in our real system and we need a guarantee that there is not transaction present at that moment.

In the unit test, Book 4 is only found when a lookup is performed in the same thread. When the lookup is carried out in the a separate thread thread, foundBook4 is set to false and it eventually fails the unit test. If you look for Book 4 in the database, you will not find it.

The biggest issues we see here is that after this specific sequence of events, anything that is processed on this particular thread will not be persisted to the database. We believe that this process can be failed if it is not executing as it should but other independent processes on the same thread should not fail or end up with missing data.

[spring-projects-issues]

Stéphane Nicoll commented

I read the test backwards. This is more clear now, thanks. I guess the main problem is that you're playing with transactions in the transaction callback, which you are not suppose to do by the way (TransactionSynchronization#afterCompletion):

 * Invoked after transaction commit/rollback.
* Can perform resource cleanup <i>after</i> transaction completion.
* <p><b>NOTE:</b> The transaction will have been committed or rolled back already,
* but the transactional resources might still be active and accessible. As a
* consequence, any data access code triggered at this point will still "participate"
* in the original transaction, allowing to perform some cleanup (with no commit
* following anymore!), unless it explicitly declares that it needs to run in a
* separate transaction. Hence: <b>Use {@code PROPAGATION_REQUIRES_NEW}
* for any transactional operation that is called from here.</b>

The problem is that your code calls a method that does impact the current thread because you mark the thread as not supporting transaction all the sudden. I'll do some homework to see if what you're experiencing can happen in a "regular" case but if not I would ask you to isolate your transactional operation there in a transaction with REQUIRES_NEW propagation (and only there). Just as required by the javadoc of the callback actually.

[spring-projects-issues]

Aldrin Seychell commented

We are now aware that we misused the transactions synchronisation callback. In fact, we fixed this issue in our code by not misusing it in this way. However, we believe that the transactions state maintained by Spring should not be corrupted by such a misuse and in fact there was no indication in the server logs that there was a misuse and the state got corrupted. This made the symptom of disappearing data difficult to track down to this particular scenario.

We believe that the Spring framework should protect its state from becoming corrupted as a result of an API misuse. The attached patch (which we already applied to our copy of the spring libraries) prevents that the Spring's state from becoming corrupt. All our tests using this patch were successful which gave us peace of mind to push this patch to our production systems.

[spring-projects-issues]

Aldrin Seychell commented

I just tested this issue with 4.3.15 and it seems to have been fixed. Thanks!

[spring-projects-issues]

Juergen Hoeller commented

Indeed, this seems to be covered by #16214 as of 4.3.7. Good to hear!