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

@EventListener annotated bean cannot be removed from the ApplicationEventMulticaster #26638

Closed
birnbuazn opened this issue Mar 4, 2021 · 8 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@birnbuazn
Copy link

Affects: 2.4.3

According to the JavaDoc of the ApplicationEventMulticaster I was expecting to be able to remove and/or add ApplicationListeners at runtime.

This currently does not work for @EventListener annotated methods, which apparently lead to a ApplicationListenerMethodAdapter instance at runtime.

Would be great, if we could remove this listener as well, either by

  1. making the Adapter instance injectable and then remove it with multicaster.removeApplicationListener(), or
  2. giving the Adapter instance a well defined bean name and remove it with multicaster.removeApplicationListenerBean()

I've included a small demo app with a failing test to illustrate what I'm trying to achieve:

event-sample.zip

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 4, 2021
@snicoll
Copy link
Member

snicoll commented Mar 4, 2021

This is consistent with the whole annotation model I am afraid. Message listeners (JMS, Rabbit, Kafka, etc.), scheduled methods, event listeners all add an aspect to the annotated method that represents its registration.

If you want to be able to remove a listener at runtime, you should create it programmatically IMO.

Can you share a bit more about your use case?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Mar 4, 2021
@birnbuazn
Copy link
Author

birnbuazn commented Mar 4, 2021

We are building an event-based modulith. Modules are currently separated by different Java packages, are deployed within the same Spring (Boot) context, and - to enforce loose coupling - communicate with each other using ApplicationEvents.

This works all surprisingly well, but we want to test our modules in isolation. I know that we could fire up different Spring contexts for the different tests and mock all non-relevant modules, but this is rather heavyweight and will slow down our tests considerably compared to firing up the Spring Boot context just once before the whole test suite runs.

So the intention was to dynamically disable all EventListeners of modules not relevant for a particular test, to avoid side-effects.

We btw. successfully went down this route by subclassing Spring's SimpleApplicationEventMulticaster with our own ConfigurableApplicationEventMulticaster (and some ReflectionUtils tricks), which we are using when running the tests, but I would love to see that adding/removing application listeners works using the official public Spring API - no matter, if the listener was created using an annotation or programmatically.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 4, 2021
@birnbuazn
Copy link
Author

Posted an answer on SO with the current workaround by subclassing Spring's SimpleApplicationEventMulticaster: https://stackoverflow.com/a/66488532/287138

@snicoll
Copy link
Member

snicoll commented Mar 5, 2021

Thanks for sharing more details. I am not keen to register a bean for every annotated method, it looks quite invasive and unnecessary. Besides, it really is an implementation details. This would also add a number of beans to the context for the sole purpose of what you're asking for.

Rather than working on the multicaster level, I wonder if building the feature in the listener itself wouldn't be better. I don't know if you're using condition already but using a meta-annotation where the condition is set to something you could control in your tests could be an option. Or some sort of abstraction where the listener would switch off itself in certain test scenarios.

I've flagged for team attention to see what the rest of the team thinks.

@snicoll snicoll added for: team-attention in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed for: team-attention status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 5, 2021
@snicoll snicoll modified the milestones: 5.x Backlog, General Backlog Mar 5, 2021
@snicoll
Copy link
Member

snicoll commented Mar 5, 2021

After a quick chat with Juergen we agreed that the asymmetry in the API is annoying and we'd like to do something about it. We'd like to explore an option where an @EventListener can have an identifier, the same way @JmsListener does. Once an id is provided we'd offer an improved api to handle the listener.

@birnbuazn
Copy link
Author

Thanks for that, that is good news.

I was also thinking, that you don't have to register the listeners as extra beans and that an identifier of some sort would be enough: You just have to extend the listener lookup in ApplicationEventMulticaster's public add and remove methods, so that it can work with this id as well.

Regarding your other suggestions: These options were on the table and they probably would work, but they are mere workarounds and we honestly don't want to bloat our production code just for testing purposes.

@jhoeller jhoeller self-assigned this Mar 5, 2021
@jhoeller jhoeller modified the milestones: General Backlog, 5.3.5 Mar 5, 2021
@jhoeller
Copy link
Contributor

jhoeller commented Mar 8, 2021

I've introduced an ApplicationEventMulticaster.removeApplicationListeners(Predicate<ApplicationListener<?>>) method in combination with ApplicationListener.getListenerId(), derived from the optional @EventListener.id attribute and defaulting to the signature of the annotated method. The latter two have been pulled up from TransactionalApplicationListener and @TransactionalEventListener where they had similar semantics already (recently introduced in 5.3, so not widely known).

I hope this works for your use case, e.g.:
removeApplicationListeners(l -> l.getListenerId().startsWith("myrootpackage.mymodule"))

jhoeller added a commit that referenced this issue Mar 8, 2021
@jhoeller
Copy link
Contributor

jhoeller commented Mar 9, 2021

I've revised this a bit to only expose the getListenerId() method on SmartApplicationListener (and also on GenericApplicationListener which derives from it now), avoiding potential conflicts with plain ApplicationListener implementations that happen to implement other interfaces as well (keeping the base interface as minimal as possible).

This means that a downcast will be necessary, I hope that's still not too bad:

removeApplicationListeners(l -> l instanceof SmartApplicationListener &&
        ((SmartApplicationListener) l).getListenerId().startsWith("myrootpackage.mymodule"))

This was referenced Mar 17, 2021
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
…lled up from tx)

Includes removeApplicationListeners(Predicate) method in ApplicationEventMulticaster.

Closes spring-projectsgh-26638
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants