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

It is not possible to have multiple handlers @EventPattern() for the same event #3074

Closed
alfredoperez opened this issue Sep 30, 2019 · 17 comments

Comments

@alfredoperez
Copy link

Bug Report

It is not possible to have multiple handlers @EventPattern() for the same event

Current behavior

Creating multiple handlers for the same event does not call all of them

Input Code

  @EventPattern('notify')
  eventHandler(data: any) {
    KafkaController.IS_NOTIFIED = data.value.notify;
  }

  @EventPattern('notify')
  secondEventHandler(data: any) {
    KafkaController.IS_NOTIFIED_TWICE = data.value.notify;
  }

Expected behavior

Both event handlers should be called.
This is needed because an application can have different features/area that has to do some work after an event happen. Eg. We deleting a customer in a shopping class we could have an OrderController that will react to "delete-customer" event and archive orders. Also, we could have a UsersController that will delete user data.

Possible Solution

There is a part of the code in NestJS that finds the handler and returns a single element. That can be changed to return all the handlers matching the event.

Environment

Nest version: 6.7

@alfredoperez alfredoperez added the needs triage This issue has not been looked into label Sep 30, 2019
@alfredoperez
Copy link
Author

I am familiar with the code and I can work on a fix. I want to know if you consider this a bug or is working as intended.

I came across this problem when working on an implementation of a service and we have the need to have multiple handlers per event.

If this doesn't get fixed I will have to roll my own implementation for Kafka.

@kamilmysliwiec
Copy link
Member

I am familiar with the code and I can work on a fix. I want to know if you consider this a bug or is working as intended.

It actually is intended. Wouldn't it be easier to have a one single entry point for your event and multiple services that are being used inside a controller?

@alfredoperez
Copy link
Author

Most of the times it should be with a single entry point, but what about situations where an event can affect a set of features(modules) .
Instead of having a god controller, any controller/feature interested can subscribe to the event and do their part of work.

It becomes easier to separate once that feature becomes it's own service if it grows to that.

Also, since it is an event I think it make sense to have any number of subscribers/handlers.

@kamilmysliwiec
Copy link
Member

Instead of having a god controller, any controller/feature interested can subscribe to the event and do their part of work.

So that may cause issues for the request-scoped controllers. Basically, since every event will have a different "entry point" (different controller's handler), for each controller different request-scoped providers tree will be created. Assuming that there is a request-scoped provider used by both controllers (from 2 features subscribing to the event), each controller will get an exclusive instance of this provider (it would be impossible to share request-scoped context). Static providers would work fine though :)

Either way, if you think that might be useful for you, I'm guessing it might be useful for other people as well. Feel free to create a PR, I'd love to take a look & review and hopefully merge shortly :)

@theabuitendyk
Copy link
Contributor

@kamilmysliwiec @alfredoperez Is there any work being done on this? We'll need support for multiple event handlers for the same event for our kafka implementation as well. Happy to contribute if needed.

@alfredoperez
Copy link
Author

@theabuitendyk I used to have a fix for this, but I don't have it at the moment.

I will look for it and get back to you

@theabuitendyk
Copy link
Contributor

@alfredoperez I'm looking at how I can add support for this to nest as we're on a tight schedule. I'll make a PR if/when I have a working solution, but would still like to see what you have. Any luck tracking it down?

@theabuitendyk
Copy link
Contributor

@alfredoperez You can disregard my last comment. :) I have a working solution now, although it's one that doesn't involve the same @EventPattern being used twice in the same app.

@gabdusch
Copy link

gabdusch commented Jul 9, 2020

Has there been any progress on this? As @alfredoperez mentioned it would be useful to have multiple event handlers for the same event in separate feature modules, with each hander only taking care of calling the services that concern that particular module. This could also be an option to be turned on/off either globally or at the module/controller level.

@kruegernet
Copy link

So that may cause issues for the request-scoped controllers. Basically, since every event will have a different "entry point" (different controller's handler), for each controller different request-scoped providers tree will be created. Assuming that there is a request-scoped provider used by both controllers (from 2 features subscribing to the event), each controller will get an exclusive instance of this provider (it would be impossible to share request-scoped context). Static providers would work fine though :)

@kamilmysliwiec, respectfully, this sounds like an edge case implementation challenge blocking the adoption of an expected behavior. If the architecture can't support multiple consumers of an event, you're not 'publishing' the event, you're sending a message (even if you don't wait for a reply). In other words, if you can't have multiple subscribers, the word 'publish' does not apply, and you don't actually have an Event Bus, so it will increases issues of excessive coupling as well as issues of circular dependency when trying to work around this shortfall.

@kruegernet
Copy link

@kamilmysliwiec, it is actually stated in the docs (Microservices/Redis):

A single message can be subscribed to (and received) by multiple subscribers.

It appears that in the code that handleEvent() is called for both message patterns and event patterns, so isn't the documentation actually incorrect here? After the first handler is registered, any other handler for the same pattern added via server.addHandler() just overwrites the last callback, so in fact only the last handler for a given pattern is the one that handles it...

@kamilmysliwiec
Copy link
Member

@kamilmysliwiec, it is actually stated in the docs (Microservices/Redis):
A single message can be subscribed to (and received) by multiple subscribers.

Subscriber = Redis subscriber (separate application instance) in this context.

@erescobar
Copy link

erescobar commented Jan 28, 2021

Hi. Any update on this issue?

@kamilmysliwiec
Copy link
Member

Let's track this here #6334

@ramyhhh
Copy link

ramyhhh commented Jan 27, 2022

Multiple event handlers is still calling only ONE handler in case if a global interceptor presence.
I've made sure next.handle() is called event with zero logic in the interceptor and still got the same behaviour.
Of course if interceptor is removed everything works find and all event pattern handlers are called.
Also it works if subscribed manually next.handle().subscribe(...) but that's very smelly workaround.

@FahmyChaabane
Copy link

FahmyChaabane commented May 8, 2022

Hello :
I believe this might answer some of your questions : https://stackoverflow.com/questions/10620976/rabbitmq-amqp-single-queue-multiple-consumers-for-same-message

@yuvall-cyera
Copy link

Multiple event handlers is still calling only ONE handler in case if a global interceptor presence. I've made sure next.handle() is called event with zero logic in the interceptor and still got the same behaviour. Of course if interceptor is removed everything works find and all event pattern handlers are called. Also it works if subscribed manually next.handle().subscribe(...) but that's very smelly workaround.

I created an issue for that #10184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants