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

unhandled 'error' event in node #18

Closed
jergason opened this issue Oct 10, 2014 · 16 comments

Comments

@jergason
Copy link
Contributor

commented Oct 10, 2014

When making a request to a url that doesn't exist, axios crashes the server on an unhandled 'error' event.

var axios = require('axios');

axios.post('http://google.com').then(function(response) {
    console.log('woo a response', response);
  })
  .catch(function(response) {
    console.log('hrrm, an error', repsonse);
  });

// needed to prevent node from immediately exiting without actually making the request
setTimeout(function() {
  console.log('what in the world, why isn\'t this waiting to exit')
}, 5000);

It looks like the problem is no req.on('error') handler on https://github.com/mzabriskie/axios/blob/1d6430f667486ca9de390ccec242114b36c41377/lib/adapters/http.js#L80.

I'm not sure the best way to handle an error here though, since it doesn't fit the expected {data, status, headers, config} type of message the reject handler expects.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 13, 2014

@jergason good catch. I can add the error handler. Not sure how the error fits in the reject either.

A couple options:

  1. Add an error property to the response object
  2. Just provide the error object directly to reject

In either case you're going to have to check if an error exists:

// option 1
.catch(function (res) {
  if (!res.data && res.error) {
    console.log('error', res.error);
  } else {
    console.log('it worked!');
  }
});

// option 2
.catch(function (res) {
  if (res instanceof Error) {
    console.log('error', res);
  } else {
    console.log('it worked!');
  }
});
@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 14, 2014

I keep stewing on this, but still don't love either option, and haven't come up with anything else better. Stay tuned...

@kentcdodds

This comment has been minimized.

Copy link

commented Oct 14, 2014

Maybe I'm misunderstanding things, but if they're using .catch then the request didn't "work" and there shouldn't be data correct? I mean the response can come back with the reason for the error, but that could be considered the error right? So res will always be an error, right?

@ryanflorence

This comment has been minimized.

Copy link

commented Oct 14, 2014

I would expect catch to reject with an error object.

@jmdobry

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2014

The issue is combing thrown errors and an http response into a single argument.

request.js example:

var request = require('request');
request('http://www.google.com', function (error, response, body) {
  if (!error && response.statusCode == 200) {
    console.log(body) // Print the google web page.
  }
})

In this example it's obvious which argument would be an Error object, caused by some exception in the code execution.

The response argument can be inspected separately, as it's status code might represent an error from a different domain, unrelated to our code's execution.

With axios:

.catch(function (res) {
  // what is res?
  // was there an exception? 
  // can I print a stack trace?
  // or do I just need to inspect the status code for 500, etc.?
});

The question is whether res will be the first or second (or both) argument from the first example. An Error object makes sense if an exception occurred, with a stack trace captured. But like @jergason said, it doesn't fit with {data, status, headers, config} which corresponds to the response argument in the first example.

A few options:

  • Always pass an Error object and requiring the developer to know how to inspect it for exception/stacktrace vs response meta data
  • Pass either an Error object or the response meta data, allowing the dev just check for which one
  • (An extension of #1) Create custom error class(es) (inheriting from Error) and expose them on axios' API. Example below:
.catch(function (err) {
  if (err instanceof axios.HttpError) {
    // determined by status code, no stack trace
    // handle non 2xx/3xx response
  } else if (err instanceof Error) {
    // caused by exception in code
    // handle error from code
  } else {
    // shouldn't get here?
  }
});

Another solution would be to follow how request.js does it (not how $http does it), as in don't decide for the developer that a particular http response should cause the promise to be rejected. In request.js, only exceptions in the code populate that error argument.

</spiel>

@kentcdodds

This comment has been minimized.

Copy link

commented Oct 14, 2014

Well explained @jmdobry... That last comment was interesting, but I think I'd recommend against going that direction for two reasons 1) you'd be forced to check the status code in your successful requests which would be annoying (especially in cases where you don't care if it was unsuccessful) 2) a lot of people use and love $http, so they'll expect axios to work similarly.

That second argument is a bit weak I think, but I thought I'd mention it anyway.

I think something like this would be reasonable:

.catch(function (result) {
  if (result.error) {
    // caused by exception in code
    // handle error from code
  } else {
    // determined by status code, no stack trace
    // handle non 2xx/3xx response
    console.log(result.data); // what came back from the server.
  }
});
@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 15, 2014

@jmdobry request's API doesn't apply to axios, since request is using a single callback to handle success/error. Versus using then/catch a la Promises. Also Promises limit then/catch to a single argument, so whether I like request's API or not, it can't be done the same way with axios.

I think I like just rejecting with the Error:

.catch(function (res) {
  if (res instanceof Error) {
    console.log(res.message);
  } else {
    console.log(res.status, res.data);
  }
});

@mzabriskie mzabriskie closed this Oct 15, 2014

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 15, 2014

Actually it looks like a PR introduced this pattern already:

https://github.com/mzabriskie/axios/blob/master/lib/adapters/http.js#L35

Anyone have any heartburn over leaving it this way? I'd prefer not to change an API that people may already depend on in order to resolve this issue.

@mzabriskie mzabriskie reopened this Oct 15, 2014

@kentcdodds

This comment has been minimized.

Copy link

commented Oct 15, 2014

I think that it's a totally reasonable/sensible solution 👍

@jmdobry

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2014

@mzabriskie Yes of course. I was just using request's api as a contrasting example to clearly define this issue by showing what axios can't do. I should have been clearer in my comment.

@ryanflorence

This comment has been minimized.

Copy link

commented Oct 15, 2014

I'm not doing as much catching up as I should, but the value sent to reject should contain more than just an error (I wasn't very clear before).

Very often you'll send a non 2xx status code but still send data. You also may need the headers, etc.

I don't think you can just throw an error into the arg and call it good, there's a lot of information about the response you may need in the handler.

@kentcdodds

This comment has been minimized.

Copy link

commented Oct 15, 2014

I think that's what @mzabriskie is suggesting with:

.catch(function (res) {
  if (res instanceof Error) {
    console.log(res.message); // this was a js error
  } else {
    console.log(res.status, res.data); // this was a non 2xx or 3xx response status
  }
});
@ryanflorence

This comment has been minimized.

Copy link

commented Oct 15, 2014

seems fine to me then

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 15, 2014

Yes @kentcdodds sums up the approach correctly.

.catch(function (res) {
  if (res instanceof Error) {
    // In this case a request was never sent to the server
    // Something happened in setting up the request that triggered an Error
  } else {
    // Here the request was made, but the server responded with a status code
    // that falls out of the range of 2xx
    // You will have the full response details available
    console.log(res.data); // The data that the server responded with
    console.log(res.headers); // The response headers from the server
    console.log(res.status); // The response status code
    console.log(res.config); // The config that was used to make the request
  }
});
@kentcdodds

This comment has been minimized.

Copy link

commented Oct 15, 2014

👏 totally reasonable api. Thanks again for making this thing dude.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 15, 2014

Thanks everyone for the dialog. I made the change as discussed and it is now on npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.