Skip to content

Loading…

remove "finish" event domain.dispose() codes. #9

Open
wants to merge 1 commit into from

4 participants

@fengmk2

No need to call dispose() on 'finish' event.

@fengmk2

It will let connect.static broken.

@baryshev
Owner

I can't reproduce this bug on a real world example. Simple app with connect-static middleware and the same behaviour works fine.

@alaendle

Don't know if this is the same problem. But I had the problem that in my case connect only responded to the first request - the second request hangs. After commenting out the dispose of the domain like requested in this issue thinks started to work as expected. Unfortunately I couldn't reproduce the problem on a small example - only my production app (which uses https://github.com/flatiron/resourceful and a MongoDb) the problem occurred.

So I wonder if I now build in something like a resource leak because I did no longer dispose the domain. Or is disposal unnecessary here?

@rprieto

We also have problems with that line. Everything works locally, but every single requests hangs when deployed to Heroku behind SSL. This our (very basic) middleware redirect.

function sslMiddleware(req, res, next) {
    if (req.headers['x-forwarded-proto'] != 'https') {
        res.redirect('https://' + req.headers["host"] + req.url);
    } else {
        next();
    }
}

If I remove res.on('finish', function () { reqDomain.dispose(); }); from connect-domain, everything works fine again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 14, 2013
  1. @fengmk2
Showing with 13 additions and 3 deletions.
  1. +4 −3 lib/connect-domain.js
  2. +8 −0 test/domain.test.js
  3. +1 −0 test/fixtures/foo.js
View
7 lib/connect-domain.js
@@ -8,9 +8,10 @@ module.exports = function () {
reqDomain.dispose();
});
- res.on('finish', function () {
- reqDomain.dispose();
- });
+ // Won't work with `connect.static()` middleware
+ // res.on('finish', function () {
+ // reqDomain.dispose();
+ // });
reqDomain.on('error', function (err) {
// Once a domain is disposed, further errors from the emitters in that set will be ignored.
View
8 test/domain.test.js
@@ -45,6 +45,7 @@ describe('domain.test.js', function () {
var app = connect()
.use(connectDomain())
+ .use('/public', connect.static(__dirname + '/fixtures'))
.use(normalHandler)
.use(errorHandler);
@@ -54,6 +55,13 @@ describe('domain.test.js', function () {
.expect(200, done);
});
+ it('should GET /public/foo.js status 200', function (done) {
+ request(app)
+ .get('/public/foo.js')
+ .expect('console.log(\'bar\');')
+ .expect(200, done);
+ });
+
it('should GET /sync_error status 500', function (done) {
request(app)
.get('/sync_error')
View
1 test/fixtures/foo.js
@@ -0,0 +1 @@
+console.log('bar');
Something went wrong with that request. Please try again.