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

Fix unhandled promise rejections when a trigger errors #1983

Closed
wants to merge 1 commit into from

Conversation

abrenneke
Copy link

When a trigger, such as the callback to controller.hears(...) errors (the async callback rejects), this promise is not handled correctly, and results in an unhandled promise rejection.

This is because the run function for ware does not support promises (the middleware themselves do, but not the callback).

I don't really agree with the decision to use a non-promise-supporting library like ware in this very promise-based framework, but we can at least promisify those calls.

This ends up simplifying a lot of the code that uses the middleware, as Promise constructors no longer need to be used.

With this fix, the promises bubble up all the way to the handleTurn function, and can be handled at the web layer.

Maybe a controller.on('error', () => {}) event would make sense as well. Or an error middleware?

@ghost
Copy link

ghost commented Jun 19, 2020

CLA assistant check
All CLA requirements met.

@abrenneke
Copy link
Author

abrenneke commented Jun 22, 2020

Now that I think about this a bit more, it seems based on the behavior of ware that middleware can't abort the request by not calling next(). It will continue the request regardless, not sure if that's intended behavior. They can throw an error, I suppose.

@alex-ubitec
Copy link
Contributor

I've encountered similar problems as well.

@benbrown benbrown closed this in 5066fb0 Aug 25, 2020
@alex-ubitec
Copy link
Contributor

That PR doesn't fix the problem.

Please take a look at the promises that this PR tries to change, because they are simply written incorrectly. The suggestion to use promisify is just to prevent those errors from propping up again, but you could just wrap the code inside the new Promise calls with a try-catch as well.

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

Successfully merging this pull request may close these issues.

None yet

2 participants