Skip to content
This repository has been archived by the owner. It is now read-only.

Error handling patterns #21

Closed
aoberoi opened this issue Mar 3, 2017 · 1 comment
Closed

Error handling patterns #21

aoberoi opened this issue Mar 3, 2017 · 1 comment
Labels

Comments

@aoberoi
Copy link
Member

@aoberoi aoberoi commented Mar 3, 2017

Description

We need to make a decision on the error handling pattern for cases where there are no event handlers attached to the error event. There are a number of variables and cases at play.

Let's consider this case:

slackEvents.on('message', event => event.foo.bar());

The handler throw a TypeError into the middleware, which will attempt to emit an error event on the adapter (slackEvents). Since there is no handler for that event type, the .emit() call will throw in the middleware. Let's take the case where there is no next middleware defined (the default if you are using the all-in-one API). The final handler from Express will respond with a 500 Internal Server Error. Express will also print the stacktrace of the error to stdout. Then, Slack will attempt reties over and over with the same result until the retries are exhausted and eventually your event subscription may be turned off on the Slack app.

This is not ideal because as the app developer, you may or may not have any indication that something is going wrong. You will see the same event hit your server three times, you will not get the result you expect, and you'll wonder what actually happened with the response. If you were listening to the error event, or listening to the proposed retry event, or using the middleware with an express application that had another error handler and chose to use the propagateErrors option, or you used the includeHeaders option and noticed the retry headers; then you would have more information about what is happening.

Another option is to "swallow" the error in the middleware and send a 200 OK response regardless. This would result in the event only showing up at your server once. But this doesn't really help us surface the error.

The EventEmitter (which SlackEventAdapter inherits from) in node core has a convention where if there is no error handler defined, then an error is thrown, with the intention that if it is not caught at the top level scope, you get one final chance to handle it in process's uncaughtException event before terminating the process. Since SlackEventAdapter is an EventEmitter, and that's the only interface a user of the all-in-one API would ever know, this feels like a strong contender for the convention this package should follow. This is at odds with Express' opinion of "keep the server up, just respond with 500 on unhandled errors".


Another distinct question is how propagateErrors should relate to the error handling system. Specifically, if propagateErrors is enabled, and the error is given to the next middleware, should the adapter also emit the error? Given the tricky nature of what happens with emitting an error, it seems simpler if propagating errors is mutually exclusive with handling them on the EventEmitter interface.

OPINIONS PLEASE!

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

Requirements

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi aoberoi added the discussion label Mar 3, 2017
@aoberoi
Copy link
Member Author

@aoberoi aoberoi commented Mar 3, 2017

after a couple offline conversations, it seems like there is no right answer here, but here are the decisions:

  1. errors thrown inside handlers when there is no error handler defined should throw (outside the middleware) and terminate the process. (#25)
  2. propagateErrors option will have mutually exclusive semantics with the error handler. (#26)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.