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

Emit error instead of throw #1558

Merged
merged 1 commit into from May 11, 2015

Conversation

Projects
None yet
5 participants
@simov
Copy link
Member

commented May 1, 2015

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

This comment has been minimized.

Copy link
Member

commented May 4, 2015

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

This comment has been minimized.

Copy link
Member Author

commented May 4, 2015

@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

This comment has been minimized.

Copy link

commented May 4, 2015

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

This comment has been minimized.

Copy link
Contributor

commented May 4, 2015

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

This comment has been minimized.

Copy link

commented May 4, 2015

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

This comment has been minimized.

Copy link
Member

commented May 5, 2015

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

This comment has been minimized.

Copy link

commented May 5, 2015

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

This comment has been minimized.

Copy link
Member Author

commented May 5, 2015

@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

This comment has been minimized.

Copy link

commented May 5, 2015

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

This comment has been minimized.

Copy link
Member Author

commented May 5, 2015

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

This comment has been minimized.

Copy link

commented May 5, 2015

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

This comment has been minimized.

Copy link
Member Author

commented May 8, 2015

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.

simov added a commit that referenced this pull request May 11, 2015

Merge pull request #1558 from simov/emit-errors
Emit error instead of throw

@simov simov merged commit 2ec7d9b into request:master May 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.