TransactionSynchronizationManager - throw an Exception or log a warning if a Synchronization wants to add a Synchronization and afterCompletion is already called [SPR-11590]

[spring-projects-issues]

Torsten Krah opened SPR-11590 and commented

Hi,

i've got a service A which registered a TransactionSynchronization to do stuff in the afterCommit() case.
It calls a service B to index things to lucene.
I've forgot that i did already leveraged the transaction handling to the lucene service B, so i did wonder why nothing happened and no error was thrown.

After debugging i've found that A did register a Synchronization (doAfterCommit) to call B, and B did also register one (doAfterCompletion).

The one which B registered was never called because doAfterCommit is the last trigger to be called and triggerAfterCompletion was already run.

Maybe a check can be added for addSynchronization(), that it throws an Exception, if the afterCompletion trigger is already reached - or at least a log warning would be nice.

---

Affects: 4.0.2

Issue Links:

• Prevent corrupted ThreadLocals when mis-using triggerAfterCommit [SPR-11761] #16383 Prevent corrupted ThreadLocals when mis-using triggerAfterCommit ("is duplicated by")
• Topic messages are not sent when using transacted JmsTemplate in 'TransactionSynchronization.afterCommit' phase [SPR-16270] #20817 Topic messages are not sent when using transacted JmsTemplate in 'TransactionSynchronization.afterCommit' phase
• Spring does not clean up db connection registered in afterCompletion callback [SPR-15194] #19759 Spring does not clean up db connection registered in afterCompletion callback
• Transaction synchronization hook - beforeCreatePhase [SPR-13724] #18297 Transaction synchronization hook - beforeCreatePhase
• Rollback of transaction participating in nested transaction should not enforce rollback of global transaction [SPR-6568] #11234 Rollback of transaction participating in nested transaction should not enforce rollback of global transaction

[spring-projects-issues]

Archie Cobbs commented

This is a more general problem that we have also seen.

Transaction synchronizations are very convenient, so convenient in fact that we've had bugs where code that is running within a synchronization invokes some other code which invokes some other code which itself registers a synchronization. This is a classic programmer "booby trap" that is easy to fall into. The result is that the latter synchronization never runs, because it's too late (the PlatformTransactionManager has already grabbed a snapshot of the list of synchronizations and is currently working through that list).

This problem is exacerbated by the fact that TransactionSynchronizationManager.isSynchronizationActive() returns true even while synchronization callbacks are being invoked. So any code that asks the question "Can I do this in an after-commit callback?" gets a "yes" answer, even though the answer is really "no".

What's worse is that the failure mode is for the callback to be silently ignored.

This should all be tightened up, both in specification and implementation. In other words, re-entrant synchronization registration behavior is not documented (it should be), and the (undocumented) behavior is sub-optimal (it should be improved).

My recommendation:

• Add a new configuration variable to AbstractPlatformTransactionManager called enforceStrictCallbacks or whatever, default false.
• Create a new package-private, thread-local boolean in TransactionSynchronizationManager called disableCallbacks or whatever.
• If someone invokes TransactionSynchronizationManager.registerSynchronization() when TransactionSynchronizationManager.disableCallbacks is set to true, throw an IllegalStateException.
• When AbstractPlatformTransactionManager.enforceStrictCallbacks is false, do the current behavior (for backward compatibility).
• However, when AbstractPlatformTransactionManager.enforceStrictCallbacks is true, have AbstractPlatformTransactionManager set TransactionSynchronizationManager.disableCallbacks for the duration of its callback processing.
• Document all of this in the AbstractPlatformTransactionManager and TransactionSynchronizationManager API javadoc.

[spring-projects-issues]

Juergen Hoeller commented

AbstractPlatformTransactionManager clears TransactionSynchronizationManager's synchronizations right before triggering afterCompletion now, with TransactionSynchronizationManager.isSynchronizationActive() returning false at that point and registerSynchronization therefore naturally rejecting any further synchronizations.

[chenxi-null]

Juergen Hoeller commented

AbstractPlatformTransactionManager clears TransactionSynchronizationManager's synchronizations right before triggering afterCompletion now, with TransactionSynchronizationManager.isSynchronizationActive() returning false at that point and registerSynchronization therefore naturally rejecting any further synchronizations.

@jhoeller
I don't understand why do you useTransactionSynchronizationManager.clearSynchronization(); to clear synchronizations, which causing #isSynchronizationActive returning false and #isActualTransactionActive returning true.

Someone may use #isActualTransactionActive to determine if current thread is in a transaction.

Could you please tell me why don't use TransactionSynchronizationManager.clear() which making #isSynchronizationActive and #isActualTransactionActive both return false ?

Thanks.