mikeal/request — Issue #171: Error Handling for Building HTTP Clients on Request
Author: dscape
Created: 2012-02-15 13:34:11+00:00
State: open

I've been giving this a lot of thought lately.

One of my favorite things in `request` is the way it handles requests. If there is a callback, stuff gets put in my callback. If there's no callback `request` returns a stream I can pipe or do whatever I want with.

One of my least favorite parts of `request` as someone that has build multiple HTTP clients on it is exception handling. Handling things in `try catch` when I have specified a callback annoys me a little, but having no predictability when I don't specify a callback is worse.

The way I fixed this in my apis is by introducing the following error handling code, and placing every call to `request` in a `try catch`:

``` js
var EventEmitter = require('events').EventEmitter;

exports= function err(error, callback) {
if(typeof error === 'string') {
error = new Error(error);
}
if(callback) {
return callback(error);
} else {
var em = new EventEmitter();
process.nextTick(function() { em.emit('error', error); });
return em;
}
};
```

This way when you are using a HTTP client I made you can be ensured that:

* If you specified a callback any error will be returned in the first argument on that callback. No exceptions.
* If you didn't specify a callback you can still intercept the error event in the stream you are probably trying to pipe. If you don't attach an error handler to your steam, you will get an `uncaughtException`.
* It maintains the same important characteristic that if your node application isn't expecting an error and doesn't know how to handle it your process will crash.

I would like to make a case for this to be the default behavior in request. It's a pretty easy fix (replacing `throws` with `err(error, callback)` and introducing some try catches in places that can potentially throw. Would like to get some feedback from others using request to validate this idea first.

For the sake of the conversation here is how exceptions are handled in request today:

```js
var request = require('request');

// Good Request with a Callback
request.get('http://writings.nunojob.com', function(err,res,bod) {
console.log(res.statusCode); // OK
});

// Good Request without a Callback
var stream = request.get('http://writings.nunojob.com');
stream.on('response', function(res) {
console.log(res.statusCode); // OK
});

// Bad Params with a Callback
// Throws
// Opinion: Should return it in the first arg of the callback.
request.get('mailto:duh', function(err,res,bod) {
console.log("Not here"); // Never gets here
});

// Bad Params without a Callback
// Throws
// Opinion: Should log Not here.
var stream = request.get('mailto:duh');
stream.on('error', function(err) {
console.log('Not here'); // Never gets here
});

// Timeout Request with a Callback
request.get('http://ithinkimighttimeout.su', function(err,res,bod) {
console.log(err); // Error here
});

// Timeout Request without a Callback
// Throws
// Opinion: Should log the error
var stream = request.get('http://ithinkimighttimeout.su');
stream.on('error', function(err) {
console.log(err); // throws? was expecting this to catch, but ok
});

// Timeout Request without a callback and error handling
// Throws
// Opinion: it should throw
request.get('http://ithinkimighttimeout.su');
```