mauricemach / zappa

Node development for the lazy.
zappajs.org
MIT License
952 stars 81 forks source link

Error handling customization #52

Closed ilyabo closed 13 years ago

ilyabo commented 13 years ago

How can I customize the error handling in zappa, e.g. to render a specific view in case if an unexpected exception is thrown? In express this can be done with app.error(), but it does not seem to work in zappa.

In the default setting zappa does not even respond to the client when an Error is thrown in a 'get' handler. Moreover, it completely stops working and will not serve any subsequent requests.

mauricemach commented 13 years ago

In zappa 0.2.x, you have complete access to express through the app variable at the root scope, so you can use app.error as usual.

You can also use the same error middleware as the default app generated by the express command, but with a shorter syntax:

configure
  development: -> use errorHandler: {dumpExceptions: on, showStack: on}
  production: -> use 'errorHandler'

If you're on 0.1.x, there's a migration guide at /docs/migration.md.

mauricemach commented 13 years ago

Actually, in 0.1.x express is also available, but at app().http_server. So you have access to app().http_server.error and app.().http_server.use, which are fugly but should work.

ilyabo commented 13 years ago

Thanks Maurice!

I am on 0.2.0beta, but changing the configuration here does not make any difference:

require('zappa') ->

  configure
    development: -> use errorHandler: {dumpExceptions: on, showStack: on}
    production: -> 'errorHandler'

  get '/': ->
    throw new Error "Oops"

  view errorHandler: ->
    p -> 'Noop'
mauricemach commented 13 years ago

Looking at the default app generated by express (/examples/express.coffee), I realized you also need to use your app.router before the exception handling middleware. This example works:

require('../src/zappa') ->
  configure ->
    use app.router

  configure
    development: -> use errorHandler: {dumpExceptions: on, showStack: on}
    production: -> 'errorHandler'

  get '/': ->
    throw new Error "Oops!"
mauricemach commented 13 years ago

I intend to provide optional means to eliminate stupid boilerplate like this in the future, but for 0.2.0 final I think we should concentrate on the short syntax for the standard express API.

ilyabo commented 13 years ago

Thanks Maurice! This works indeed.

I realized though that my real problem is with this code:

require('zappa') ->

  def mysql = require('mysql').createClient(
    user: 'root'
    password: ''
    database: 'studies'
  )

  get '/': ->
    load_studies -> 
        throw new Error
        render 'home'

  def load_studies:(callback) ->
    mysql.query \
      'SELECT 1',
      (err, results) ->
        client.end
        callback()

  view home: ->
    p -> 'Hello'

  view layout: -> body -> @body

I call load_studies() which reads something from a database and give it a callback to be invoked on success. If an exception is thrown in this callback, then this message is output in the console and the whole application STOPS WORKING:

  undefined:4
          throw new Error;
                ^
Error
    at eval at <anonymous> (/home/boyandii/node_modules/zappa/lib/zappa.js:32:12)
    at eval at <anonymous> (/home/boyandii/node_modules/zappa/lib/zappa.js:32:12)
    at Query.<anonymous> (/home/boyandii/node_modules/mysql/lib/client.js:108:11)
    at Query.emit (events.js:61:17)
    at Query._handlePacket (/home/boyandii/node_modules/mysql/lib/query.js:51:14)
    at Client._handlePacket (/home/boyandii/node_modules/mysql/lib/client.js:309:14)
    at Parser.<anonymous> (native)
    at Parser.emit (events.js:64:17)
    at /home/boyandii/node_modules/mysql/lib/parser.js:71:14
    at Parser.write (/home/boyandii/node_modules/mysql/lib/parser.js:576:7)

It is totally fine that the error message is shown, but the web server should continue to answer other requests which is not the case. I see that it happens in the mysql module, but could I do something to prevent zappa from crashing in this case?

mauricemach commented 13 years ago

This happens even when you setup your error handling middleware that way? Can't reproduce here, express shows the error page and another route keeps responding correctly.

ilyabo commented 13 years ago

Yes, the error handling middleware does not make any difference.

If I move the error out of the callback:

get '/': ->
    throw new Error()
    load_studies -> 
           render 'home'

then zappa continues to run after the error. Otherwise, if it is in the callback, it stops without sending any response to the browser.

I use the latest node-mysql: 0.9.2

mauricemach commented 13 years ago

Found it, this is a node/express issue, due to the asynchronous nature of node. One solution is to catch the error and pass it to next.

ilyabo commented 13 years ago

Thanks, Maurice! It makes sense.

ilyabo commented 13 years ago

I noticed that zappa still crashes with the following error:

Error
    at eval at <anonymous> (/home/boyandii/node_modules/zappa/lib/zappa.js:32:12)
    at eval at <anonymous> (/home/boyandii/node_modules/zappa/lib/zappa.js:32:12)
    at Query.<anonymous> (/home/boyandii/node_modules/mysql/lib/client.js:108:11)
    at Query.emit (events.js:61:17)
    at Query._handlePacket (/home/boyandii/node_modules/mysql/lib/query.js:51:14)
    at Client._handlePacket (/home/boyandii/node_modules/mysql/lib/client.js:309:14)
    at Parser.<anonymous> (native)
    at Parser.emit (events.js:64:17)
    at /home/boyandii/node_modules/mysql/lib/parser.js:71:14
    at Parser.write (/home/boyandii/node_modules/mysql/lib/parser.js:576:7)

http.js:527
    throw new Error("Can't set headers after they are sent.");
          ^
Error: Can't set headers after they are sent.
    at ServerResponse.<anonymous> (http.js:527:11)
    at ServerResponse.setHeader (/home/boyandii/node_modules/zappa/node_modules/express/node_modules/connect/lib/patch.js:50:20)
    at /home/boyandii/node_modules/zappa/node_modules/express/node_modules/connect/lib/middleware/errorHandler.js:72:19
    at [object Object].<anonymous> (fs.js:107:5)
    at [object Object].emit (events.js:61:17)
    at afterRead (fs.js:878:12)
    at wrapper (fs.js:245:17)
[Finished]

If there is a render after next(new Error):

get '/': ->
    load_studies ->
          next(new Error())
          render 'home'

If I remove render 'home', then it only shows the error and does not crash

mauricemach commented 13 years ago

You shouldn't call next with the error and render a page, it's one thing or the other, like in this example. More specifically in your case, something like:

get '/': ->
  load_studies (err, results) ->
    if err then next(err)
    else render 'home'          

You'll get this error whenever you try to call next, render, send etc twice, because the headers were already sent to the client and it doesn't make sense to set them after that. Many platforms have a similar error (PHP's "cannot modify headers information - headers already sent"). Node previously just failed silently in this case, but not anymore.

In summary, as far as I understand, when building node apps, you should always be watching out for errors in callbacks you should try and catch yourself (and pass to next in the case of express/connect), and be careful not to send responses twice. Yeah, node error handling in web apps is not very intuitive at the moment.

Anyway, FYI, as a last resort, there's always process.on 'uncaughtException'. You don't have information from the context where the error was generated, so you cannot send the user an appropriate response (your site will hang and timeout to them) but at least your server will keep running and you'll be able to log uncaught errors.

mauricemach commented 13 years ago

Speaking of uncaughtException, there are [more gotchas]((http://debuggable.com/posts/node-js-dealing-with-uncaught-exceptions:4c933d54-1428-443c-928d-4e1ecbdd56cb) to watch out for.

ilyabo commented 13 years ago

To my mind this uncaughtException callback should be enabled by default in any server app, but this is obviously a node issue.

Thanks for the great explanation!