Emit error instead of throw

[simov]

Related to this #1318 (comment) onward

Breaking change

Any code that previously relied on catching the errors in the catch block will now receive them in the callback as errors if a callback is specified

request({uri:'...'}, function (err, res, body) {
  if (err) {
    // all errors are received here
  }
}).on('error', function (err) {
  // that's being called before being registered
})

No callback during initialization

This repository still have two error cases that throws in index.js But if the callback is missing, any error will be thrown anyway

try {
  request({uri:'...'}).on('error', function (err) {
    // that's being called before being registered
  })
} catch (err) {
  // EventEmitter throws the error if the event is unhandled
}

After successful initialization

request({uri:'...'}, function (err, res, body) {
  if (err) {
    // all errors are received here
  }
}).on('error', function (err) {
  // after successful initialization this is being registered
})

---

One possible way to receive the errors during initialization in the on('error') event handler, is to use this hack

setTimeout(self.emit.bind(self, 'error', new Error('...')), 0)
// instead of
self.emit('error', new Error('...'))

But it's unreliable and I'm not going to use it.

// cc @mikeal @nylen @FredKSchott @saper

[nylen]

I actually like the idea of emitting all errors to the error event handler for consistency, and you could use process.nextTick instead of setTimeout. It's not ideal, but would triggering the error async actually cause any issues? @simov what did you mean by "it's unreliable"?

[simov]

@nylen setTimeout is unreliable because it doesn't guarantee when exactly it will be executed, although in most cases it should work just fine (it's 4ms at minimum). Now that I'm reading about it nextTick seems to have better performance because it doesn't involve a timer.

In our code base we have this wrapper used only at the end of the init method.

So we can have this code but it looks ugly, and in some cases it might cause issues I think.

defer(self.emit.bind(self, 'error', new Error('...')))
// instead of
self.emit('error', new Error('...'))

[ziggythehamster]

When I do error events before they can be listened in my code (i.e., constructors), I use process.nextTick to emit the error so that I have time to listen for the error.

Either all events/errors are emitted asynchronously, or they're thrown synchronously. Do not mix synchronous and asynchronous or you will release Zalgo.

If you do decide to throw/emit synchronously (where you can't .on('error')), can you please give us a flag to set that would allow us to opt-in to asynchronous errors?

[FredKSchott]

I see the value in both behaviors, but I prefer what we have currently to this. The thinking is that errors that occur on initialization are caused by bad input / are programmer errors and should get more exposure, while async errors related to redirects and responses are expected errors (something can always go wrong).

[saper]

I was trying to do something like that (naively):

var r = require('request')

r.on('error', function(er, {console.error('got ya!')}).get('badurl.com')

And get 'got ya' called on invalid URL for example.

You can export a module which behaves like EventEmitter. But EventEmitter constructor makes sure that event listener table in a new object instance.

If you are okay with one bit ugly copy operations of the _events array, you can try this solution:

simov#2

and it can be used as follows:

var request = require('request')
request.on('init', function(er) {console.log('starting')})
request.on('initialized', function(er) {console.log('ready to go')})
request.on('error', function(er) {
    console.log('first error handler')
})
var z = request.on('error', function(er) { 
    console.log('second error handler')
}).get('aaa');

which produces:

starting
first error handler
second error handler
ready to go

https://gist.github.com/saper/67d2bd6a6cd81ee9773a

I think this is a pretty crazy idea but may be we can achieve something useful this way.

[nylen]

I see the value in both behaviors, but I prefer what we have currently to this. The thinking is that errors that occur on initialization are caused by bad input / are programmer errors and should get more exposure, while async errors related to redirects and responses are expected errors (something can always go wrong).

You could work around this by emitting any initialization errors before adding the default .on('error') handler for the callback. That way they have to be handled or there's an uncaughtError.

crazy idea with module-level .on('error')

This feels a little too "magic" to me.

My other concern with making changes here - are we creating situations where we return a broken, half-initialized object?

[saper]

On Mon, 4 May 2015, James Nylen wrote:

I see the value in both behaviors, but I prefer what we have currently to this. The thinking is that errors that occur on initialization are caused by bad input / are programmer errors and should get more exposure, while async errors related to redirects and responses are expected errors (something can always go wrong).

You could work around this by emitting any initialization errors before adding the default .on('error') handler for the callback. That way they have to be handled or there's an uncaughtError.

crazy idea with module-level .on('error')

This feels a little too "magic" to me.

Attaching a handler to an initialized Request obviously does not work... Given that we often face situation
where a whole URL is a configuration parameter, environment variable or worse user input I'd rather
have a general catch-all wrapper for initialization errors.

My other concern with making changes here - are we creating situations where we return a broken, half-initialized object?

I think yes, but that's what we do now anyway:

• on module level a working EventEmitter interface is returned by require('request'). So no half-broken object here.
• on request level the situation stays the same as it was (callbacks, instance handlers get called)
• if no callbacks/handlers are provided emit() will raise an exception as it does today, so "new"
  fails.
• if there is a callback and Request.init() fails via self.emit() the error will be passed
  on to the callback and a partially usable object can be returned, but only after attempting
  the request.

This is the situation we have right now I think with the case:

https://gist.github.com/saper/8ddf5f180f848379bc47

var request = require('request')
var z = request.get('aaa', function(e) {
 console.log('error');
});
console.log('Half-broken object', z.uri)

where we get with the current master:

error
Half-broken object { protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: null,
  search: null,
  query: null,
  pathname: 'aaa',
  path: 'aaa',
  href: 'aaa' }

We never return an object straight from the constructor
before making the request, do we?

[simov]

@saper your solution is clever, but modifying the global state is extremely bad practice. The problem is that you're not using the new keyword. I'm giving you a short example (run app.js)

[saper]

Of course this is expected. We are simply doing

 var r = require('request').on('error', function /*.. */)

There is no way such a handler would magically be bound to the Request instance. It's a per-module handler, so it's neither file1 nor file2 nor callx based.

request() is a factory function itself, so introducing another new makes no sense, unless we really want one event emiter per require('request') invocation...

I just had a look at node's http module and they solve it nicely: _http_client.ClientConnect is synchronous and throws exceptions, while _http_agent.Agent is event-driven.

[simov]

It is expected for single error event to pop up on various places through the entire app, and multiple times - that's some weird expectations TBH :)

[saper]

I have to agree :)

well, I never really expected we will go this way - it was just a funny exercise in JavaScript.

But I agree nextTick is a horrible solution and it's better to say with exceptions.

[simov]

Ok, so this tiny PR makes the error behavior consistent only when you have a callback. I'm glad we raised the awareness about the error related issue, but like I said a few comments above, I'm not sure about the best way to wrap the emit statements, so if there aren't any other suggestions I'm going to merge this PR shortly.