Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spring-issuemaster opened this issue Mar 24, 2014 · 3 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Mar 24, 2014

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:

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

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 13, 2016

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 9, 2017

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

This comment has been minimized.

Copy link

commented Oct 18, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.