Detect invalid transaction configuration for transactional event listeners

[odrotbohm]

Problem

It's common practice to use a simple (no custom attributes) @Transactional on methods to execute them wrapped in a transaction. Unfortunately, for methods declared as @TransactionalEventListener, this doesn't work as expected:

@Component
class SomeListener {

  @Transactional
  @TransactionalEventListener
  void on(SomeEvent event) { /* … */ }
}

Those methods are invoked in the transaction cleanup phase of an original, other transaction. This means that this particular, other transaction is still running, although in an undefined state. With a simple @Transactional declared on those methods, the code would be executed in an undefined transactional state. An obvious solution to this problem is to declare a Propagation of REQUIRES_NEW on the listener method, so that it will run in a new transaction:

@Component
class SomeListener {

  @Transactional(propagation = Propagation.REQUIRES_NEW)
  @TransactionalEventListener
  void on(SomeEvent event) { /* … */ }
}

Proposed solution

It would be nice if the erroneous arrangement shown above would be rejected, ideally at bootstrap (potentially even AOT?) time. We have to consider that a plain @Transactional is still valid in the case of the method declared to be invoked asynchronously (using @Async). In other words, a declaration like this is still valid, as the asynchronous invocation triggers the execution on a separate thread and thus a clean slate regarding transactions.

@Component
class SomeListener {

  @Async
  @Transactional
  @TransactionalEventListener
  void on(SomeEvent event) { /* … */ }
}

[quaff]

After 842569c

@Component
class SomeListener {

  @Transactional(propagation = Propagation.REQUIRES_NEW)
  @TransactionalEventListener
  void on(SomeEvent event) { /* … */ }
}

will not be valid, is it intentional? @jhoeller

[jhoeller]

Good point, the check seems too restrictive now. I'll revise it to be lenient towards REQUIRES_NEW.

[jhoeller]

I'm inclined to enforce REQUIRES_NEW here since that is semantically correct for such a listener in general. Even with @Async execution, it would be self-descriptive to always declare REQUIRES_NEW there. Also, our check in the transaction package cannot actually enforce actual async execution, it can only check for the presence of the common @Async annotation without knowing whether the corresponding runtime setup is active.