Handling errors gracefully #36

Open
redsquirrel opened this Issue Jul 13, 2011 · 22 comments

Comments

Projects
None yet
@redsquirrel

This may not be an issue, it may just be me not understanding express and/or everyauth well enough yet. But, it seems like the default error behavior will cause an uncaughtException, which will either let the process crash, or can be caught with process.on("uncaughtException"...), but without a response to write to, the process will hang.

Are there established techniques for handling errors gracefully when everyauth is handling the req/res lifecycle?

@bnoguchi

This comment has been minimized.

Show comment
Hide comment
@bnoguchi

bnoguchi Jul 15, 2011

Owner

Ctrl-f "Configuring Error Handling" in the README.

Quoted:

By default, all modules handle errors by throwing them. That said, everyauth allows
you to over-ride this behavior.

You can configure error handling at the module and step level. To handle all
errors in the same manner across all auth modules that you use, do the following.

everyauth.everymodule.moduleErrback( function (err) {
  // Do something with the err -- e.g., log it, throw it
});

You can also configure your error handling on a per module basis. So, for example, if
you want to handle errors during the Facebook module differently than in other modules:

everyauth.facebook.moduleErrback( function (err) {
  // Do something with the err -- e.g., log it, throw it
});
Owner

bnoguchi commented Jul 15, 2011

Ctrl-f "Configuring Error Handling" in the README.

Quoted:

By default, all modules handle errors by throwing them. That said, everyauth allows
you to over-ride this behavior.

You can configure error handling at the module and step level. To handle all
errors in the same manner across all auth modules that you use, do the following.

everyauth.everymodule.moduleErrback( function (err) {
  // Do something with the err -- e.g., log it, throw it
});

You can also configure your error handling on a per module basis. So, for example, if
you want to handle errors during the Facebook module differently than in other modules:

everyauth.facebook.moduleErrback( function (err) {
  // Do something with the err -- e.g., log it, throw it
});

@bnoguchi bnoguchi closed this Jul 15, 2011

@redsquirrel

This comment has been minimized.

Show comment
Hide comment
@redsquirrel

redsquirrel Jul 15, 2011

Yep, I've read that. But without a response to write to, it doesn't help me respond gracefully. Do you know what I mean? My only option is to re-throw an Error. But I can't get the express.error callback to work.

Yep, I've read that. But without a response to write to, it doesn't help me respond gracefully. Do you know what I mean? My only option is to re-throw an Error. But I can't get the express.error callback to work.

@bnoguchi

This comment has been minimized.

Show comment
Hide comment
@bnoguchi

bnoguchi Jul 15, 2011

Owner

Ah, I see. Let me work on getting you access to res from within the moduleErrback callback.

Owner

bnoguchi commented Jul 15, 2011

Ah, I see. Let me work on getting you access to res from within the moduleErrback callback.

@bnoguchi bnoguchi reopened this Jul 15, 2011

@bnoguchi

This comment has been minimized.

Show comment
Hide comment
@bnoguchi

bnoguchi Jul 15, 2011

Owner

I just committed a change that should allow you to do the following:

.moduleErrback( function (err, data) {
  var res = data.res;
  // Do what you want with `data` and `res` from here
});

Try using the everyauth repo with npm via npm link

Owner

bnoguchi commented Jul 15, 2011

I just committed a change that should allow you to do the following:

.moduleErrback( function (err, data) {
  var res = data.res;
  // Do what you want with `data` and `res` from here
});

Try using the everyauth repo with npm via npm link

@redsquirrel

This comment has been minimized.

Show comment
Hide comment
@redsquirrel

redsquirrel Jul 15, 2011

Awesome! Thanks! But where'd you commit that change? I don't see it anywhere, and I'm not sure how to "npm link" would help. Any pointers?

Awesome! Thanks! But where'd you commit that change? I don't see it anywhere, and I'm not sure how to "npm link" would help. Any pointers?

@bnoguchi

This comment has been minimized.

Show comment
Hide comment
@bnoguchi

bnoguchi Jul 15, 2011

Owner

Sorry. git pull origin master now. npm link enables you to work with the module without having to npm install. You'd want to use this because the change you need is not yet published in a new version of everyauth to npm.

Owner

bnoguchi commented Jul 15, 2011

Sorry. git pull origin master now. npm link enables you to work with the module without having to npm install. You'd want to use this because the change you need is not yet published in a new version of everyauth to npm.

@bnoguchi

This comment has been minimized.

Show comment
Hide comment
@bnoguchi

bnoguchi Jul 15, 2011

Owner

Let me know if that works.

Owner

bnoguchi commented Jul 15, 2011

Let me know if that works.

@redsquirrel

This comment has been minimized.

Show comment
Hide comment
@redsquirrel

redsquirrel Jul 15, 2011

Looks like it's working great. Thanks loads!

Looks like it's working great. Thanks loads!

@ricardobeat

This comment has been minimized.

Show comment
Hide comment
@ricardobeat

ricardobeat Aug 28, 2011

.moduleErrback doesn't seem to be receiving the data argument. I'm using v0.2.18.

.moduleErrback doesn't seem to be receiving the data argument. I'm using v0.2.18.

@leisms

This comment has been minimized.

Show comment
Hide comment
@leisms

leisms Sep 21, 2011

on v0.2.20 and I can't access data.res inside .moduleErrback either, for any module

leisms commented Sep 21, 2011

on v0.2.20 and I can't access data.res inside .moduleErrback either, for any module

@mavarley

This comment has been minimized.

Show comment
Hide comment
@mavarley

mavarley Oct 6, 2011

It seems as though if the exception was produced with a Promise.fail() that the

   catch (breakTo) { 
       if (..) {
           :
       } else {
           errorCallback (breakTo, seq.values);
       }
     }

block in step.js is not executed, but rather the Promise.errback(errorCallback) method is called further on. The result is the moduleErrorback(..) method gets called, but without the data parameter.

I have encountered this while trying to gracefully handle 'Authentication cancelled' errors in the googlehybrid / openid module.

Any suggestions on how to properly catch/handle these errors would be greatly appreciated.

mavarley commented Oct 6, 2011

It seems as though if the exception was produced with a Promise.fail() that the

   catch (breakTo) { 
       if (..) {
           :
       } else {
           errorCallback (breakTo, seq.values);
       }
     }

block in step.js is not executed, but rather the Promise.errback(errorCallback) method is called further on. The result is the moduleErrorback(..) method gets called, but without the data parameter.

I have encountered this while trying to gracefully handle 'Authentication cancelled' errors in the googlehybrid / openid module.

Any suggestions on how to properly catch/handle these errors would be greatly appreciated.

@quickredfox

This comment has been minimized.

Show comment
Hide comment
@quickredfox

quickredfox Oct 17, 2011

@mavarley

What I did to move ahead in the mean time ( edit line 23 everyauth/lib/step.js )

errorCallback = this.errback || function(error){
         _module._moduleErrback.call(this, error, seq.values)
}

@mavarley

What I did to move ahead in the mean time ( edit line 23 everyauth/lib/step.js )

errorCallback = this.errback || function(error){
         _module._moduleErrback.call(this, error, seq.values)
}
@zfedoran

This comment has been minimized.

Show comment
Hide comment
@zfedoran

zfedoran Nov 4, 2011

This issue should be reopend.

I cannot access data.res either when using Promise.fail().

zfedoran commented Nov 4, 2011

This issue should be reopend.

I cannot access data.res either when using Promise.fail().

@jondot

This comment has been minimized.

Show comment
Hide comment
@jondot

jondot Nov 27, 2011

I'm also stuck on this. It's a mismatch between connect/express ability to trap errors and everyauth's. I can't afford to catch on process level either - because i can't end requests.

jondot commented Nov 27, 2011

I'm also stuck on this. It's a mismatch between connect/express ability to trap errors and everyauth's. I can't afford to catch on process level either - because i can't end requests.

@bnoguchi bnoguchi reopened this Nov 27, 2011

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 20, 2012

is there any chance of this going into master / npm package soon? My specific scenario is our API requires oAuth UID + email, with twitter obviously being a pain. So I would like to catch the error and redirect the user to a form requesting an email.

ghost commented Jan 20, 2012

is there any chance of this going into master / npm package soon? My specific scenario is our API requires oAuth UID + email, with twitter obviously being a pain. So I would like to catch the error and redirect the user to a form requesting an email.

@symposion

This comment has been minimized.

Show comment
Hide comment
@symposion

symposion Feb 5, 2012

To add an additional perspective to this, even the proposed solution here seems to be sub-optimal. Express/Connect have a well defined set of mechanisms for handling errors within asynch middleware: you call next(err). This conforms to node best practice for asynch error handling and allows central error handlers to be registered that display errors to the user in a consistent way throughout the application.

Everyauth's "step" system is essentially a mini-middleware solution of its own but it seems like it isn't properly integrated with the surrounding Connect middleware; it never calls next() at all, with or without an error. This may seem a bit radical, but I think a better solution might be to dispense entirely with the everyauth error callback system; instead it should capture broken promises/error returns/exceptions and pass them through to next(err) to be handled by the normal Express error handling routines.

If this is too radical you could leave the errorHandlers there as a reporting mechanism but surround them with try-catch and then implement something along the lines suggested above to actually control flow through the middleware.

If this doesn't sound deeply objectionable to you/anyone else commenting here I'd be happy to make some changes and submit a pull request - I think it should be a fairly simple series of changes in stepSequence.js/step.js .

To add an additional perspective to this, even the proposed solution here seems to be sub-optimal. Express/Connect have a well defined set of mechanisms for handling errors within asynch middleware: you call next(err). This conforms to node best practice for asynch error handling and allows central error handlers to be registered that display errors to the user in a consistent way throughout the application.

Everyauth's "step" system is essentially a mini-middleware solution of its own but it seems like it isn't properly integrated with the surrounding Connect middleware; it never calls next() at all, with or without an error. This may seem a bit radical, but I think a better solution might be to dispense entirely with the everyauth error callback system; instead it should capture broken promises/error returns/exceptions and pass them through to next(err) to be handled by the normal Express error handling routines.

If this is too radical you could leave the errorHandlers there as a reporting mechanism but surround them with try-catch and then implement something along the lines suggested above to actually control flow through the middleware.

If this doesn't sound deeply objectionable to you/anyone else commenting here I'd be happy to make some changes and submit a pull request - I think it should be a fairly simple series of changes in stepSequence.js/step.js .

@symposion

This comment has been minimized.

Show comment
Hide comment
@symposion

symposion Feb 6, 2012

This turned out to be pretty simpler so I've made the pull request already: #173.

This turned out to be pretty simpler so I've made the pull request already: #173.

@mavarley

This comment has been minimized.

Show comment
Hide comment
@mavarley

mavarley Feb 6, 2012

Thanks @symposion, I'll have a look.

mavarley commented Feb 6, 2012

Thanks @symposion, I'll have a look.

pepkin88 added a commit to pepkin88/everyauth that referenced this issue Feb 20, 2012

@pepkin88

This comment has been minimized.

Show comment
Hide comment
@pepkin88

pepkin88 Feb 20, 2012

Contributor

Thanks @quickredfox, that worked for me.

Contributor

pepkin88 commented Feb 20, 2012

Thanks @quickredfox, that worked for me.

@mrw4n

This comment has been minimized.

Show comment
Hide comment
@mrw4n

mrw4n Mar 17, 2012

guys i still can't fix this, i cant handle errors.

is there a fix for that?

mrw4n commented Mar 17, 2012

guys i still can't fix this, i cant handle errors.

is there a fix for that?

@madhums

This comment has been minimized.

Show comment
Hide comment
@madhums

madhums Jun 2, 2012

Still unable to access the data.res in moduleErrback. I am using v0.2.32

madhums commented Jun 2, 2012

Still unable to access the data.res in moduleErrback. I am using v0.2.32

@briemens

This comment has been minimized.

Show comment
Hide comment
@briemens

briemens Jul 27, 2012

Two months further and no update. v0.2.32 is still the last version on NPM! What is blocking to release a fix? I can patch it myself, but I would like to keep in sync with the master branch and NPM releases.

Two months further and no update. v0.2.32 is still the last version on NPM! What is blocking to release a fix? I can patch it myself, but I would like to keep in sync with the master branch and NPM releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment