baryshev / connect-domain Goto Github PK

I found that this middleware is causing my Express.js 4/Node.js 0.12 based app to halt after a number of concurrent requests.

Didn't find the time to investigate more. Please tell me if you need more information, as I am seeing that this repo hasn't updated for a long time.

Has anyone experienced the same issue with other implementation of Node.js Domain module?

Thank you.

Running on localhost, connect-domain v0.5.

/Users/alandu/web-projects/ft/web/node_modules/connect-domain/lib/connect-domain.js:17
			reqDomain.dispose();
			          ^

TypeError: reqDomain.dispose is not a function
    at Domain.<anonymous> (/Users/alandu/web-projects/ft/web/node_modules/connect-domain/lib/connect-domain.js:17:14)
    at Domain.emit (events.js:315:20)
    at Domain.EventEmitter.emit (domain.js:485:12)
    at Domain._errorHandler (domain.js:253:23)
    at Object.<anonymous> (domain.js:156:29)
    at process._fatalException (internal/process/execution.js:164:29)

If the request handler is asynchronous (and anything other than a timer, which is cleaned up specially in domain.dispose() - an http request for a slow resource is a good example), and the client closes its connection (calling domain.dispose to be called, which calls res.end()), then when our request handler's async method returns, it runs in the app's domain, not the request domain (which no longer exists), and any calls to res.writeHead() will now cause an app-level crash (saying headers have already been written).

Fix is probably to just not call domain.dispose() in response to 'close' on the response object.

According to the domain module docs (and common sense), in case of an unhandled exception the process should stop accepting new connections and eventually shut down.

A sane default behavior would be to stop listening on http port(s) and do a process.exit from setTimeout with a reasonable delay (30 seconds?), plus an event to add any app-specific shutdown logic.

(node:10742) [DEP0012] DeprecationWarning: Domain.dispose is deprecated. Recover from failed I/O actions explicitly via error event handlers set on the domain instead.

When recovering from an uncaughtException, one may want to restart the server (left in an "unstable" state). The problem is that your middleware does not "mark" errors redirected to the connect error handler, so there is no way to tell the difference between an error voluntarily passed to next(err) and an uncaughtException legitimately caught by connect-domain.

One solution would be to decorate the error with a custom Error type, so that the connect error handler could restart when errors come from connect-domain.

This is a so convoluted error that took me a while to discover.. It is an specific combination of modules (request and connect-mongodb).

Here is the smallest version I did:

//npm install express request mongodb connect-domain connect-mongodb

var request = require('request'),
    express = require('express'),
    connectDomain = require('connect-domain'),
    http = require('http');

var MongoStore = require('connect-mongodb'), 
    Db = require('mongodb').Db, 
    Server = require('mongodb').Server, 
    server_config = new Server('localhost', 27017, {auto_reconnect: true, native_parser: true}), 
    db = new Db('test', server_config, {}),
    mongoStore = new MongoStore({db: db});

var app = express();

app.configure(function(){
  //if i move this line after express.session 
  //everything works as expected.
  this.use(connectDomain());

  this.use(express.cookieParser());
  this.use(express.session({secret: 'foobar', store: mongoStore }));

  this.use(app.router);

  this.use(function(err, req, res, next){
    res.end(err.message);
  });
});

app.get('/', function(req, res, next){
  var r = request.get('http://localhost:9999');
  //at this point r.domain is null.

  //If i explicitly add this stream to the domain it works
  //Slightly changing connect-domain to store the domain in req, if i do 
  //req.__domain.add(r); it works
  r.pipe(res);
});

http.createServer(app).listen(3000, function(){
  console.log('listening on http://localhost:3000');
});

The official Node docs say we should shutdown the process after receiving an error with domains:

  var server = require('http').createServer(function(req, res) {
    var d = domain.create();
    d.on('error', function(er) {
      console.error('error', er.stack);

      // Note: we're in dangerous territory!
      // By definition, something unexpected occurred,
      // which we probably didn't want.
      // Anything can happen now!  Be very careful!

      try {
        // make sure we close down within 30 seconds
        var killtimer = setTimeout(function() {
          process.exit(1);
        }, 30000);
        // But don't keep the process open just for that!
        killtimer.unref();

        // stop taking new requests.
        server.close();

        // Let the master know we're dead.  This will trigger a
        // 'disconnect' in the cluster master, and then it will fork
        // a new worker.
        cluster.worker.disconnect();

        // try to send an error to the request that triggered the problem
        res.statusCode = 500;
        res.setHeader('content-type', 'text/plain');
        res.end('Oops, there was a problem!\n');
      } catch (er2) {
        // oh well, not much we can do at this point.
        console.error('Error sending 500!', er2.stack);
      }
    });

    // Because req and res were created before this domain existed,
    // we need to explicitly add them.
    // See the explanation of implicit vs explicit binding below.
    d.add(req);
    d.add(res);

    // Now run the handler function in the domain.
    d.run(function() {
      handleRequest(req, res);
    });
  });
  server.listen(PORT);
}

How can this be adapted to connect-domain? I have some simple skeleton code I started with, but it is missing some critical pieces that I don't understand:

var express = require('express');
var app = express();
var connectDomain = require('connect-domain');

app.use(express.static('/public'));
app.use(connectDomain());

app.get('/', function(req, res) {
  res.send('Hello World!');
});

// This is where I would handle process shutdown
app.use(function(err, req, res, next) {
  try {
    // Adapting nodejs process shutdown here
    var killtimer = setTimeout(function() {
      process.exit(1);
    }, 30000);
    // But don't keep the process open just for that!
    killtimer.unref();


    // ??? How to close server ???
    // server.close();


    // try to send an error to the request that triggered the problem
    res.statusCode = 500;
    res.setHeader('content-type', 'text/plain');
    res.end('Oops, there was a problem!\n');

  } catch (er2) {
    console.log('Error sending 500!, er2.stack);
  }
});

// ??? How would the following be adapted ???
/*----------------
d.add(req);
d.add(res);

// Now run the handler function in the domain.
d.run(function() {
  handleRequest(req, res);
});
---------------*/

// Start application
app.listen(3000, function() {
  console.log("Express app started!");
});

cant get it working with nested promises:
this works:

    Q.ninvoke(db, 'collection', 'users')
    .then(function(collection) {
       throw new Error('123');   // error in first level
      return Q.ninvoke(collection, 'findOne', {url: id})
       .then(function(item) {
        ...

but this don't:

    Q.ninvoke(db, 'collection', 'users')
    .then(function(collection) {
      return Q.ninvoke(collection, 'findOne', {url: id})
       .then(function(item) {
           throw new Error('123');  // error in nested promise
           ...

any help appreciated..

Hi guys, this is not exatcly an issue (but a misuse maybe), so I am creating this issue to keep documented so other folks may not struggle and get stucked like me.

Requests like myapp.com/js/jquery.js were simply hanging forever in the server not allowing my page to be fully rendered, was not even getting error 404. And the most crazy part was that sometimes it worked sometimes not. At a totally random fashion.

My code before (with problems):

app.use(connectDomain());
app.use(express.compress());
app.use(express.static(__dirname + "/static"));
app.use(express.bodyParser());
app.use(express.methodOverride());
app.use(express.cookieParser());

Then I solved by pulling connectDomain some lines down the stack:

app.use(express.compress());
app.use(express.static(__dirname + "/static"));
app.use(connectDomain());
app.use(express.bodyParser());
app.use(express.methodOverride());
app.use(express.cookieParser());

Maybe its worth adding this info at the readme file.

hello there

i think the recommended function dispose() is not called on an error. let me explain, you have this line in your code:

reqDomain.on('error', next);

this is fine, you're delegating error handling to the middleware. but there is no guarantee that the middleware is going to render the error properly nor it's going dispose of the error there. an error might be thrown there again for any reasons.

i recommend to improve this code line further to make it more stable:

reqDomain.on('error', function(err) {
    try {
        next(err);
    } catch(exc) {
        console.error('Sorry, your middleware could not handle the error %s', err);
        console.error('Reason: %s', exc);
        // tried our best. clean up anything remaining.
        reqDomain.dispose();
    }
});

this is also recommend for another reason: to avoid memory leaks within nested domains.

my code suggestion is part of the official documentation:
http://nodejs.org/docs/latest/api/domain.html

cheers
michael

Connect-domain works for me if I simulate async errors with a setTimeout(...) callback. But when I use a Sequelize data model to make an SQL query, and throw an error inside its callback, connect-domain catches the exception only once. Reloading the browser page causes Node.js to crash. Any idea what's going on?

I'm using

expressApp.use connectDomain()
... set up views that use Sequelize ...
expressApp.use express.errorHandler(dumpExceptions:true, showStack:true)

Why not just always call next(err) instead of supporting the custom handler. That way the middleware becomes simpler since you just create a domain object and capture any errors from it. You let the normal connect stack handle error propagation.