-
Notifications
You must be signed in to change notification settings - Fork 150
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
Exceptions thrown from within event handlers crashes the app #409
Comments
Would you like to create a minimal reproduction repository? This would help us target/solve this issue sooner. |
Hi @kamilmysliwiec , I apologize for the delay in responding. Below is the link to a reproduction repository: https://github.com/diego-rangel/nestjs-cqrs-crash-reproducer REPRO STEPS:
|
Any news about this issue? |
I can confirm the issue of a crashing application. |
Probably here is the problematic part Line 68 in a75efa4
We don't have any check if handler.handle(event) doesn't throw any exception. This is very likely possible to crash the app.I can fix it, but @kamilmysliwiec needs to decide the proper execution context for an exception from this place. It doesn't fit any currently supported context types since from the event bus we don't know if the event was dispatched from an HTTP request or something else. Maybe there should be a new context type for such a case in which we give the EventHandler class as Handler and the event as Args? Or maybe the event bus should have knowledge about the request context? That can be related to this issue #60 Or maybe exceptions from the event handler should be thrown as let say EventHandlerException to make it possible to have a dedicated ExceptionFilter and make it possible to store all failed events to rerun them. I'm really into this option with a dedicated context for such a case. |
Or maybe a dedicated provider like EVENT_ERROR_HANDLER which will not require a context at all? |
IMHO ExepctionFilter is a too high level for it. The handler execution may occur without any request when an event is thrown by a CRON/lifecycle hook/whatever. Another problem is we actually may don't know what the right way is to handle the error. Maybe, crashing the application is the only solution. Imagine that an error in the handler may introduce the application into an inconsistent state. The handler creator should decide what happens when the handler fails. |
Great options @Sikora00 and @Dyostiq Q On top of the above, there could be a graceful transition to require such handler in every module (i.e. to put it in Just to mention another option (not believing much in this tho but could be a nice addition to aspect-oriented programming) is dedicated decorator which could handle such. |
@kgajowy but actually there can be an event without handlers |
Hey guys. I think I'm facing the same problem with notifications about exceptions occuring outside "request/response" scope. Do you already have a recommended approach to make the error tracking more concise in these situations? |
Add log when event handler faced an error before the app crash Refs nestjs#409
Add log when event handler faced an error before the app crash Refs nestjs#409
Event handler exceptions causing application shutdown is an unexpected developer experience. Other comparable means of asynchronous processing in the nestjs ecosystem don't follow that behavior. To preserve backward compatibility, it might be worth introducing a module-level configuration for error handling that allows the community to easily implement global error handling logic or config to turn off the default crash behavior altogether. |
To preserve backward compatibility, we can also introduce a new optional method |
That sounds like a good approach to me. We could utilize something like |
Actually, I meant a method at the level of each event handler. |
Add log when event handler faced an error before the app crash Refs nestjs#409
I interpret the problem here as being: Errors within the event handlers cause the nestjs application to shut down The best way to solve my understanding of the problem and keep the module backward compatible would be to allow for a module-level optional error handler to be passed to the CqrsModule. I don't see how adding an optional method to individual event handlers allows the developer to solve my understanding of the problem easily. Isn't that virtually the same amount of work as them surrounding the innards of their handler method with a try-catch? Is my understanding of the problem correct? |
I don't think we should implement an Exception Filter. I don't think exception filters will play nicely with handling errors that occur in the observable subject. I'm suggesting that we provide a config for an optional onEventHandlerError. It will allow the developer to address the crashing issue on a global scale if they choose to do so. It'd be backward compatible, optional, and address the issue at hand. If the developer wants to handler an exception on a handler level, can't they just implement a try-catch block? I'll try to get a code example up later this week. Sorry for not providing a pr up to this point! |
@collinc777 I meant the second part. That we shouldn't handle all errors in a single place because only the event producer knows how to react to such a situation |
I'm not suggesting that we force a global error handler. I suggest that we allow the developer to add one if they choose to via an optional config. The issue of "exceptions thrown in the handler crash the application" is global by nature. It should have a global solution. |
@collinc777 I agree with you on this. There seems to be a disconnect in this thread with some who:
This issue is about (1). The app shouldn't just kill the process unexpectedly. This does not happen for anything else in the app, and we can't use Exception Filters to deal with it, because it's outside of the request pipeline. |
@Sikora00 any further updates on this? Maybe we can all start using a fork until nestjs supports this officially ? |
I would love to @kodeine. There is a fork under @nestjs-architects but just tell me what is the desired approach xD |
@Sikora00 ill start using cqrs from |
@kodeine you can find my on NestJS's Discord. Especially at DDD or architecture channel. |
I confirm the issue. I thought it's related to node version and to unhandled promise rejections: https://developer.ibm.com/blogs/nodejs-15-release-blog/ So if you do this, at least the application won't crash. process.on('unhandledRejection', (reason, promise) => { |
@mciureanu I'm not sure this is a best practice as described in this article. https://blog.heroku.com/best-practices-nodejs-errors This could be a workaround for now, but we definitely need a way to handle this kind of error at the module, even handler or sagas level. |
This behavior was introduced in @nestjs/cqrs version 8.0.4. It works as expected on version 8.0.3 |
So what SHOULD I do if I encounter this issue? I don't want my server freeze every BRE. |
@dudematthew Our solution, for now, is to use a try/catch in our event handlers' I would love to see a more holistic solution from the NestJS team. This library is a core utility in our application. I'd like to contribute if there was direction to do so. @kamilmysliwiec @Sikora00 |
hey guys, this is a very critical problem because it crashes the application even in the lastest version, i wish i could help fix it. |
@incompletude |
it is not a unhandled error. im throwing an exception that is well handled by my application. but if by handled you mean putting all my event handlers inside try catchs, yes, they are unhandled. im stuck at 8.0.3 where this behaviour doesn't exists. |
Decorating 'handle' with this: |
If you do that, the application will not crash, ok, but you will no longer be able to handle events in the handler that crashed. |
Is there any way to handle errors in Saga? |
Any news on this? I think we need to find a different way to handle this on the nestjs level. I don't think using a try-catch block is the right approach because the service can be used by multiple controllers/event handlers/sagas/cron jobs and other invokers. For example, I have a command that can be executed by both an HTTP controller and event handlers. I want to have an exception filter at the HTTP level to handle any errors that occur. If I refactor it to a try/catch block, it means I won't be able to use the exception filter functionality. Additionally, I don't want to refactor the entire application and the services that have already been written to use a try-catch block. |
the workaround I found was this (example taken from nestjs CQRS(saga) docs) Starter code: @Injectable()
export class HeroesGameSagas {
@Saga()
dragonKilled = (events$: Observable<any>): Observable<ICommand> => {
return events$.pipe(
ofType(HeroKilledDragonEvent),
map((event) => new DropAncientItemCommand(event.heroId, fakeItemID)),
);
}
} Refactored code: @Injectable()
export class HeroesGameSagas {
@Saga()
dragonKilled = (events$: Observable<any>): Observable<ICommand> => {
return events$.pipe(
ofType(HeroKilledDragonEvent),
map((event) => new DropAncientItemCommand(event.heroId, fakeItemID)),
map((cmd) => this.CommandBus.execute(cmd)), // <-- added
map(() => null), // <-- added
catchError(() => []), // <-- added
);
}
} Manually executing via command bus lets us catch error using |
In v10 we won't auto-re-throw errors = the app will simply log the unhandled exception down to the console and remain responsive #1390 |
For those looking for a way to handle & react to unhandled exceptions, check out this PR #1399. Description: Provides the private readonly destroy$ = new Subject<void>();
constructor(private readonly unhandledExceptionsBus: UnhandledExceptionBus) {
this.unhandledExceptionsBus
.pipe(takeUntil(this.destroy$))
.subscribe((exceptionInfo) => {
// Handle exception here
// e.g. send it to external service, terminate process, or publish a new event
});
}
onModuleDestroy() {
this.destroy$.next();
this.destroy$.complete();
} To filter out stream and subscribe to a specific exception type, use the this.unhandledExceptionsBus
.pipe(
takeUntil(this.destroy$),
UnhandledExceptionBus.ofType(TransactionNotAllowedException),
)
.subscribe((exceptionInfo) => {}); Payload interface: /**
* Represents an unhandled exception.
*/
export interface UnhandledExceptionInfo<
Cause = IEvent | ICommand,
Exception = any,
> {
/**
* The exception that was thrown.
*/
exception: Exception;
/**
* The cause of the exception (event or command reference).
*/
cause: Cause;
} |
When starting to deal with a type of application that receives Webhooks from an external service, my application reacts by triggering events based on the type of object received. For example, an event that triggers an email to the customer. So, when dealing with some smtp plugins, I accidentally realized that if any exception is thrown from within EventHandlers, the application completely crashes.
Current behavior
Exception thrown from within EventHandlers craches the app.
Expected behavior
A way to handle and don't crash the app.
Minimal reproduction of the problem with instructions
Environment
The text was updated successfully, but these errors were encountered: