domain.dispose() before next(err) #8

Merged
merged 1 commit into from Mar 12, 2013

Projects

None yet

2 participants

Contributor
fengmk2 commented Mar 4, 2013

No description provided.

Contributor
fengmk2 commented Mar 7, 2013

@baryshev Please review this when you're free.

Owner
baryshev commented Mar 7, 2013

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/');
Owner
baryshev commented Mar 7, 2013

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.

Contributor
fengmk2 commented Mar 8, 2013

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.

Contributor
fengmk2 commented Mar 8, 2013

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

Owner
baryshev commented Mar 8, 2013

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.

Owner
baryshev commented Mar 8, 2013

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.

Contributor
fengmk2 commented Mar 12, 2013

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

@baryshev baryshev merged commit 9066460 into baryshev:master Mar 12, 2013
@fengmk2 fengmk2 deleted the fengmk2:remove-dispose branch Mar 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment