Skip to content
This repository

Error Handling for Building HTTP Clients on Request #171

Closed
dscape opened this Issue February 15, 2012 · 26 comments

9 participants

Nuno Job Mikeal Rogers Aaron Heckmann Pedro Teixeira Isaac Z. Schlueter John Hurliman Chris O'Hara Jeff Waugh Andreas Lind Petersen
Nuno Job

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:

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:

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');
Aaron Heckmann

definitely agree that errors should be passed to the callback if exists else emitted on the stream.

Pedro Teixeira

@aheckmann yup, agree, even errors that happen synchronously.
+1

Isaac Z. Schlueter
Collaborator

Why isn't the callback just set as the stream.on('error') handler?

Mikeal Rogers
Owner

@isaacs is right.

we need to:

  • factor out all throw statements that happen after the constructor is called.
  • force all errors to emit('error') on the Request instance.
  • when the callback is present, add a safe (read: can't call the callback more than once) handler for "error".
Mikeal Rogers
Owner

also, errors that happen in the constructor should remain. it's much better to fail early on the blocking call if the arguments are invalid.

right now that approach is kind of controversial but it'll be far less controversial after domains land and invalid argument errors are much easier to debug when they throw immediately when called.

John Hurliman

So we will still need to wrap calls to request in try/catch blocks? Except in cases where all the arguments are constant of course.

Nuno Job

@jhurliman the code to fix this is not in request still.

according to @mikeal the way he see's it you still need a try/catch even if you have a callback and this fix is applied.

he feels synchronous errors should be thrown cause they are not http errors. this will be manageable with domains.

personally i have conflicting views:

  • as a developer i hate having to put a freking try catch
  • as a lib maintainer i think these errors are mostly when a developer is coding and finding them earlier is better than later. domain will intercept these and render a 500 if you like. you can then inspect the logs and fix your bug.

what do you think?

Mikeal Rogers
Owner

in general, you should throw when you're going to do something you absolutely know will fail and you're in the middle of a blocking call.

with the recent refactor we should throw in the constructor and in any composable methods that find errors. All errors related to streaming or coming from the underlying request and response objects should emit error on request.

we still need a fix to create an error listener for the callback a user passes.

Mikeal Rogers
Owner

you should send valid urls.

if you send give a valid url to request then something is wrong with your code. the question is about how soon and how obvious the error will be to you, because once you give an invalid url to request you already have a bug it's just a matter of where and how we show you.

a lot of the concerns over try/catch wrapping will be moot once we introduce domains anyway.

Isaac Z. Schlueter
Collaborator

Please don't assume too much about what domains will or won't do at this point. The goal of domains is to bind different IO actions together so that emit('error') can be caught in one place, not necessarily to make throw behave intelligently (though of course, that would be lovely, it's out of the scope).

In general, while it's still vapor, building too much based on assumptions is really hazardous.

Mikeal Rogers
Owner

so, the crux of the argument here really comes down do "try/catch is slow" which is why i won't put it in the library and why people don't want it in their application code necessarily and would like to see errors emitted. domains actually will solved that :)

Isaac Z. Schlueter
Collaborator

Yes, assuming that you use emit("error", er) rather than throw er for everything other than "bad args", then domains will make things nicer (unless we change them before then ;)

Mikeal Rogers
Owner

i'm not releasing request 3.0 until this works.

Isaac Z. Schlueter
Collaborator

@mikeal "this works" ?= #171 (comment)

Mikeal Rogers
Owner

@isaacs we know what to do someone just needs to write it :)

Nuno Job

I'm also getting uncaught exceptions on ECONNREFUSED.

Problem is described in detail here:
http://rentzsch.tumblr.com/post/664884799/node-js-handling-refused-http-client-connections

Mikeal Rogers
Owner

that is outdated, you should be able to listen only for errors on the ClientRequest object. req.socket won't be assigned until some time in the future.

Nuno Job

Me and @indexzero formalized the answer to this issue in a package caller errs.

Still I think that you are the best guy to apply this @mikeal cause no one will understand the code as well as you.

Mikeal Rogers mikeal closed this in 74ca9a4 April 07, 2012
Chris O'Hara

@mikeal when removing an option key in future (e.g. onResponse) can you please bump the minor version? Removing functionality is not a patch

Mikeal Rogers
Owner

we're way in the 100s, doing release candidates for the next major version. it's taking longer than anticipated to get that major version out so apologies stuff like this is creeping in.

Jeff Waugh
jdub commented April 18, 2012

Uh oh, does this mean the onResponse functionality is unlikely to remain in 3.x?

Chris O'Hara

+1 for adding it back

Andreas Lind Petersen
Collaborator

+1 from me also.

Andreas Lind Petersen
Collaborator

@mikeal I find the convention with the 2.9.100+ releases a little weird since eg. a dependency on eg. 2.9.x in package.json will pick them up. Shouldn't it be 3.0.0rc1, 3.0.0rc2 and so forth?

Mikeal Rogers
Owner

i'm not disagreeing with the convention being strange. but, we're in this strange place where we mostly have bug fixes and minor refactors and need the 2.9.x people to test.

not going to add onResponse back because it's identical to the response event, which is much nicer.

request.get(url).on('response', function () {})
Jeff Waugh
jdub commented April 18, 2012

Excellent! Sorry if I implied I wanted onResponse back -- it's just the functionality I cared about. And this is indeed nicer. :-)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.