nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.73k stars 7.63k forks source link

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

Closed alfredoperez closed 3 years ago

alfredoperez commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

Hi. Any update on this issue?

kamilmysliwiec commented 3 years ago

Let's track this here https://github.com/nestjs/nest/pull/6334

ramyhhh commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/nestjs/nest/issues/10184