domain.dispose() before next(err)

[fengmk2]

@baryshev Please review this when you're free.

[baryshev]

Sorry for slow response. Changing on to once may cause aplication crash if more than one errors occurred inside of request.

Simple test case:

var async = require('async');
var http = require('http');
var connect = require('connect');
var app = connect();
var connectDomain = require('connect-domain');
app.use(connectDomain());
app.use(function (req, res) {
    async.parallel([function (cb) {
        setTimeout(function () {
            throw new Error('test1');
            cb();
        }, 1000);
    }, function (cb) {
        setTimeout(function () {
            throw new Error('test2');
            cb();
        }, 1100);
    }], function () {
        res.writeHead(200, {'Content-Type': 'text/plain'});
        res.end('Hello World\n');
    });
});

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

http.createServer(app).listen(1339, '127.0.0.1');
console.log('Server running at http://127.0.0.1:1339/');

[baryshev]

About removing reqDomain.dispose(). I'm not sure that's a good idea. I think we need to add reqDomain.dispose() to error handler and hook res.end like this:

res.end = function () {
    reqDomain.dispose();
    res.end.apply(this, arguments);
};

to dispose domain in all situations. dispose removing all event listeners from domain which make garbage collector work more simpler.

[fengmk2]

We only using "Implicit Binding" mode, domain.dispose() will be auto executed.

If you want to nest Domain objects as children of a parent Domain, then you must explicitly add them, and then dispose of them later.

Only on "Explicit Binding" mode, we need to call dispose() by ourself.

[fengmk2]

If more than one errors occurred inside of request, we should only send one error response to request user.

[baryshev]

Yes, we need to send only one error to user, but if we use .once application will crash after second error in the same request. You can check this with code example from my first comment. I use node 0.8.22.

[baryshev]

domain.dispose() never called automatically. It can be checked by adding listener to domain dispose event. So if we have to cleanup unnecessary domains we need to call .dispose() at all of them.

[fengmk2]

@baryshev I rebase the code and add async error appear twice test cases.