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

Improve module integration by simplifying event listener declaration #80

Closed
odrotbohm opened this issue Nov 24, 2022 · 7 comments
Closed
Assignees
Labels
in: event publication registry Event publication registry type: enhancement Major enhanvements, new features
Milestone

Comments

@odrotbohm
Copy link
Member

tl;dr

We would like to simplify the event-based module integration and make sure the default transactional setup is correct by introducing a custom @ModuleIntegrationListener annotation.

Context

Classic Spring applications use bean references and invocations to orchestrate functionality, even if the functionality triggered resides in different application modules. In those cases, the default approach to consistency is a transaction that spans the original business method and all transitive functionality:

@Service
@RequiredArgsConstructor
class OrderApplicationService {

  private final Inventory inventory;
  private final RewardsProgram rewards;

  @Transactional
  void complete(Order order) {

    // Change state of the order and conclude unit of work
    inventory.updateStockFor(order);
    rewards.registerBonusPointsFor(order);
  }

We usually recommend replacing these kinds of interactions with letting the OrderApplicationService rather publish an event and letting the previously actively invoked components listen to that event:

@Service
@RequiredArgsConstructor
class OrderApplicationService {

  private final ApplicationEventPublisher events;

  @Transactional
  void complete(Order order) {

    // Change state of the order and conclude unit of work
    events.publishEvent(OrderCompleted.of(order.getId()));
  }
}

@Service
class Inventory {

  @EventListener
  void on(OrderCompleted event) { … }
}

@Service
class RewardsProgram {

  @EventListener
  void on(OrderCompleted event) { … }
}

This arrangement elegantly fixes the cyclic dependency between the components, simplifies testability but still keeps the consistency model of the previous approach, with all its pros and cons. However, to further decouple the modules from each other, and to limit the scope of the original business transaction, it might make sense to rather turn the event listeners, into asynchronous, transaction-bound ones.

@Service
class RewardsProgram {

  @Async
  @TransactionalEventListener
  void on(OrderCompleted event) { … }
}

This declaration will cause the even listener method to be invoked during the cleanup of the original business method's transaction (hence the name @*Transactional*EventListener).

Problem

One problem with this arrangement is that @*Transactional*EventListener might create the impression that the listener itself is transactional, which it – declared like this – is not. Users understanding that will very likely go ahead and tweak the declaration to this:

@Service
class RewardsProgram {

  @Async
  @Transactional
  @TransactionalEventListener
  void on(OrderCompleted event) { … }
}

This is quite a buit of low-level demarcation and it's easy to miss a detail about the setup. Developers might not actively consider the asynchronous execution. In that case a transactionally correct setup would actually require the transaction propagation settings to be set to REQUIRES_NEW as otherwise, the already running, committed transaction would be reused and the listener running in undefined (likely auto-commit) mode.

Solution

To simplify the declaration and prevent users from misconfiguring their transactional arrangement, it makes sense to provide a custom annotation @ApplicationModuleIntegrationListener, meta-annotated with the set of annotations shown above.

@Async
@Transactional(propagation = Propagation.REQUIRES_NEW)
@TransactionalEventListener
// … other annotations
public @interface ApplicationModuleIntegrationListener { }

@Service
class RewardsProgram {

  @ApplicationModuleIntegrationListener
  void on(OrderCompleted event) { … }
}

We could also provide verification for a variety of cases:

  • Reject @AMIL listening to a non-foreign application module's events (via ArchUnit)
  • Reject non-async transactional event listeners not using REQUIRES_NEW as propagation (via ArchUnit, at runtime)
  • @AMIL used in applications that do not have transactions activated at all (runtime)
  • @TransactionalEventListener registered for an event that's published but while no transaction is running

Consequences / Alternatives

Application code would need to use a Spring Modulith specific annotation in a very fundamental use case. Traditionally, we've tried to avoid that by making the defaults sane and only require Spring Modulith-specific annotations for specialized cases. Also, one would have to inspect the custom annotation for its meta annotations to actually understand what's going on. In other words, we would be introducing an indirection.

@odrotbohm odrotbohm added the in: event publication registry Event publication registry label Nov 24, 2022
@odrotbohm odrotbohm added this to the 0.2 milestone Nov 24, 2022
@odrotbohm odrotbohm self-assigned this Nov 24, 2022
odrotbohm added a commit that referenced this issue Dec 2, 2022
We now provide @ApplicationModuleIntegrationListener as shortcut for the combination of @async @transactional @TransactionalEventListener to provide a dedicated annotation for the recommended integration arrangement.
@odrotbohm
Copy link
Member Author

odrotbohm commented Dec 2, 2022

This is now available in the 0.2 snapshots. There's still a naming debate to be had. @ApplicationModuleIntegrationListener is quite a mouthful itself. We could switch to @ApplicationModuleListener with the following pros and cons:

Pro

  • It's more concise.

Cons

  • There's ApplicationListener in Spring Framework which can be considered a sibling naming wise, but is actually an interface. In other words, we'd create a slight usage inconsistency for classes in the "same" context (event consumption).
  • @ApplicationModuleIntegrationListener also expresses the listener's purpose: module integration, which is what we intend it to help with. @ApplicationModuleListener sounds more generic.

Shall we rename @ApplicationModuleIntegrationListener to @ApplicationModuleListener? Please use the thumbs up / down feature on this comment to vote. Additional comments for rationales and your evaluations of the pros and cons (matter, don't matter) appreciated.

@sfeldkord
Copy link

I really like the idea to simplify asynchronous event handling. It especially reduces errors introduced by missing out on any of the combined annotations. But why not name it just like that: @AsyncTransactionalEventListener?

While this is not shorter, to me it is more intuitive and straight forward, especially when migrating from synchronous event handling.

While @ApplicationModuleListener (my favorite of your proposals) raises the abstraction level, it does not tell me anything about its transactional or asynchronous nature.

@odrotbohm
Copy link
Member Author

Thanks for the feedback, @sfeldkord! @ATEL didn't make too much sense to me, as it doesn't really add any real abstraction. It's a bit too close to the originally necessary arrangement, but then omits one of the "transactional"s, which might confuse people, as they might not expect a transaction to be created for the listener.

This is probably all because @TransactionalEventListener is ambiguous in the first place. That's why I thought using a name that would summarize the purpose of the method annotated would be better. What that (an AML) actually means would then be obvious from the Javadoc (please have a look at its current state, I am open for suggestions) and the fact, that @AML is directly annotated with the lower-level annotations. After all, you could still fall back to applying them individually in case that pre-packaged combination doesn't suit your use case.

@pnedonosko
Copy link

Name like ApplicationModuleEventListener would sound more DDD-way and naturally connected to a domain event (e.g. from Event Storming design).

@odrotbohm
Copy link
Member Author

I like that as an additional option to consider. It points a bit to the fact that ApplicationModuleListener is a bit of a stretch, as you don't actually listen to the module in the first place. On the other hand, there's no explicit notion of an "application module event" though (maybe there should?), we're primarily suggesting the annotation as the default event listening setup for events crossing module boundaries, which is a rather implicit thing (event type located in another module).

@pnedonosko
Copy link

pnedonosko commented Dec 8, 2022

the fact that ApplicationModuleListener is a bit of a stretch, as you don't actually listen to the module in the first place.

yep, it sounds too general for a developer, as *Listener in Java applied to many things already, it may mean a broad set of things.

there's no explicit notion of an "application module event" though (maybe there should?),

If look at this from domain design point of view, if a concrete module treated as a bounded context, this would mean producing of an event - what already done, otherwise such need should be first clarified to avoid overloading the whole concept (in an another issue, I guess).

we're primarily suggesting the annotation as the default event listening setup for events crossing module boundaries, which is a rather implicit thing (event type located in another module).

Assuming a module as a bounded context, then we have crossing of contexts boundaries, what is a classical case of DDD and naming telling this would be obvious for the framework users: *ContextEventListener. Such name as ModuleContextEventListener would do the job, a shorten ModuleContextListener also good, but it hasn't such event notion thus may confuse a first time reader of the code/guide. As for ApplicationContextEventListener it may confuse broader as Spring itself has context and application notions.

PS: I found the Spring Modulith only yesterday evening, it's great initiative but I haven't used it yet, thus my ideas here are only opinionated by architecting and development experience in other frameworks.

@odrotbohm
Copy link
Member Author

Thanks for chiming in, Peter, especially in such tough times! There are cases in which we find a 1:1 mapping between a module and a Bounded Context, but I'd like to keep the latter out of scope here, as we primarily design the interaction between the modules. Those occasionally mapping to BC relationships is not something I'd want to put in the middle of the terminology for something primarily dealing with modules.

I think I'll be going with @ApplicationModuleListener for now as it semantically aligns with ApplicationListener already existing in Spring Framework but applies specific semantics of listening to events published by a module. We're still in the 0.x phase of the project so that we can still react and tweak things in case the feedback from the field indicates a need.

@odrotbohm odrotbohm added the type: enhancement Major enhanvements, new features label Dec 23, 2022
yossisp added a commit to yossisp/spring-modulith that referenced this issue Jun 2, 2023
…D however that would reuse the already running committed transaction which is not the correct behavior (spring-projects#80).
odrotbohm pushed a commit that referenced this issue Jun 14, 2023
The default propagation type of @transactional is Propagation.REQUIRED. That, however would reuse the already running committed transaction which is not the correct behavior.

Related ticket: #80.
odrotbohm pushed a commit that referenced this issue Jun 23, 2023
The default propagation type of @transactional is Propagation.REQUIRED. That, however would reuse the already running committed transaction which is not the correct behavior.

Related ticket: #80.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: event publication registry Event publication registry type: enhancement Major enhanvements, new features
Projects
None yet
Development

No branches or pull requests

3 participants