From 431fbb7d98525299432e3c110ebe554ca658974b Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Mon, 18 May 2020 16:37:41 +0200 Subject: [PATCH 01/23] Proxy res.end(callback) calls correctly --- index.js | 2 +- test/session.js | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 9615346c..ad90ff58 100644 --- a/index.js +++ b/index.js @@ -272,7 +272,7 @@ function session(options) { return ret; } - if (chunk == null) { + if (chunk == null || typeof chunk === 'function') { ret = true; return ret; } diff --git a/test/session.js b/test/session.js index f0b60fdf..b20da863 100644 --- a/test/session.js +++ b/test/session.js @@ -189,6 +189,23 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) + it('should handle res.end(callback) calls', function (done) { + var callbackHasBeenCalled = false; + + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } + + res.end(callback); + }); + + request(server).get('/').expect(200, '', function () { + assert.ok(callbackHasBeenCalled); + done(); + }) + }); + it('should handle res.end(null) calls', function (done) { var server = createServer(null, function (req, res) { res.end(null) From 2482cf2c6f6c4ae569acb1a2819f087f94f982e9 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Wed, 20 May 2020 14:23:21 +0200 Subject: [PATCH 02/23] work in progress, doesn't work properly --- .gitignore | 1 + index.js | 30 ++++++++++++++++++++++++----- package.json | 7 ++----- test/session.js | 51 ++++++++++++++++++++++++++++++++----------------- 4 files changed, 62 insertions(+), 27 deletions(-) diff --git a/.gitignore b/.gitignore index 207febba..615e06d4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.idea/ .nyc_output coverage node_modules diff --git a/index.js b/index.js index ad90ff58..47b3e369 100644 --- a/index.js +++ b/index.js @@ -247,7 +247,7 @@ function session(options) { var _end = res.end; var _write = res.write; var ended = false; - res.end = function end(chunk, encoding) { + res.end = function end() { if (ended) { return false; } @@ -257,9 +257,25 @@ function session(options) { var ret; var sync = true; + var endArguments = arguments; + var chunk = endArguments[0]; + var encoding = endArguments[1]; + + console.log('about to shuffle arguments', { chunk: chunk, encoding: encoding }); + + // Callback may be the 1st (and only), second, or third argument + if (typeof chunk === 'function') { + console.log('chunk was function'); + chunk = null; + encoding = null; + } else if (typeof encoding === 'function') { + console.log('encoding was function'); + encoding = null; + } + function writeend() { if (sync) { - ret = _end.call(res, chunk, encoding); + ret = _end.apply(res, endArguments); sync = false; return; } @@ -272,7 +288,7 @@ function session(options) { return ret; } - if (chunk == null || typeof chunk === 'function') { + if (chunk == null) { ret = true; return ret; } @@ -301,6 +317,7 @@ function session(options) { } if (shouldDestroy(req)) { + console.log('shouldDestroy'); // destroy session debug('destroying'); store.destroy(req.sessionID, function ondestroy(err) { @@ -317,17 +334,20 @@ function session(options) { // no session to save if (!req.session) { + console.log('has no session'); debug('no session'); - return _end.call(res, chunk, encoding); + return _end.apply(res, endArguments); } if (!touched) { + console.log('session was not touched') // touch session req.session.touch() touched = true } if (shouldSave(req)) { + console.log('should save') req.session.save(function onsave(err) { if (err) { defer(next, err); @@ -352,7 +372,7 @@ function session(options) { return writetop(); } - return _end.call(res, chunk, encoding); + return _end.apply(res, endArguments); }; // generate the session diff --git a/package.json b/package.json index ba2adb65..f0bea203 100644 --- a/package.json +++ b/package.json @@ -22,12 +22,9 @@ "devDependencies": { "after": "0.8.2", "cookie-parser": "1.4.5", - "eslint": "3.19.0", - "eslint-plugin-markdown": "1.0.2", "express": "4.17.1", - "mocha": "7.1.1", - "nyc": "15.0.1", - "supertest": "4.0.2" + "mocha": "2.5.3", + "supertest": "1.1.0" }, "files": [ "session/", diff --git a/test/session.js b/test/session.js index b20da863..961d3b37 100644 --- a/test/session.js +++ b/test/session.js @@ -189,22 +189,39 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - it('should handle res.end(callback) calls', function (done) { - var callbackHasBeenCalled = false; - - var server = createServer(null, function (req, res) { - function callback() { - callbackHasBeenCalled = true; - } - - res.end(callback); - }); - - request(server).get('/').expect(200, '', function () { - assert.ok(callbackHasBeenCalled); - done(); - }) - }); + // it('should handle res.end(callback) calls', function (done) { + // var callbackHasBeenCalled = false; + // + // var server = createServer(null, function (req, res) { + // function callback() { + // callbackHasBeenCalled = true; + // } + // + // res.end(callback); + // }); + // + // request(server).get('/').expect(200, '', function () { + // var nodeVersionStrings = process.versions.node.split('.'); + // var nodeVersion = { + // major: Number(nodeVersionStrings[0]), + // minor: Number(nodeVersionStrings[1]), + // patch: Number(nodeVersionStrings[2]) + // } + // + // console.log('nodejs version', {nodeVersion: nodeVersion }); + // + // if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // // Node versions prior to 0.11.6 do not support the callback argument + // console.log('callback should NOT be called') + // assert.ok(!callbackHasBeenCalled) + // } else { + // console.log('callback SHOULD be called') + // assert.ok(callbackHasBeenCalled) + // } + // + // done() + // }) + // }); it('should handle res.end(null) calls', function (done) { var server = createServer(null, function (req, res) { @@ -361,7 +378,7 @@ describe('session()', function(){ }) }) - it('should have saved session even with multi-write', function (done) { + it.only('should have saved session even with multi-write', function (done) { var saved = false var store = new session.MemoryStore() var server = createServer({ store: store }, function (req, res) { From fa4ba454784a8dbb6f3c76a9ae75de148867a9c2 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Fri, 22 May 2020 17:16:25 +0200 Subject: [PATCH 03/23] --wip-- [skip ci] --- index.js | 26 +++++++++++++++----------- test/session.js | 40 ++++++++++++++++++++++------------------ 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/index.js b/index.js index 47b3e369..f8452a5e 100644 --- a/index.js +++ b/index.js @@ -247,7 +247,7 @@ function session(options) { var _end = res.end; var _write = res.write; var ended = false; - res.end = function end() { + res.end = function end(c, e) { if (ended) { return false; } @@ -257,23 +257,27 @@ function session(options) { var ret; var sync = true; - var endArguments = arguments; + var endArguments = Array.from(arguments); + // var endArguments = arguments; + // var chunk = endArguments[0]; + // var encoding = endArguments[1]; var chunk = endArguments[0]; var encoding = endArguments[1]; console.log('about to shuffle arguments', { chunk: chunk, encoding: encoding }); - // Callback may be the 1st (and only), second, or third argument - if (typeof chunk === 'function') { - console.log('chunk was function'); - chunk = null; - encoding = null; - } else if (typeof encoding === 'function') { - console.log('encoding was function'); - encoding = null; - } + // // Callback may be the 1st (and only), second, or third argument + // if (typeof chunk === 'function') { + // console.log('chunk was function'); + // chunk = null; + // encoding = null; + // } else if (typeof encoding === 'function') { + // console.log('encoding was function'); + // encoding = null; + // } function writeend() { + console.log('writeend') if (sync) { ret = _end.apply(res, endArguments); sync = false; diff --git a/test/session.js b/test/session.js index 961d3b37..66b73b5d 100644 --- a/test/session.js +++ b/test/session.js @@ -342,12 +342,12 @@ describe('session()', function(){ } request(server) - .get('/') - .expect(200, 'session saved', function (err) { - if (err) return done(err) - assert.ok(saved) - done() - }) + .get('/') + .expect(200, 'session saved', function (err) { + if (err) return done(err) + assert.ok(saved) + done() + }) }) it('should have saved session even with empty response', function (done) { @@ -370,12 +370,12 @@ describe('session()', function(){ } request(server) - .get('/') - .expect(200, '', function (err) { - if (err) return done(err) - assert.ok(saved) - done() - }) + .get('/') + .expect(200, '', function (err) { + if (err) return done(err) + assert.ok(saved) + done() + }) }) it.only('should have saved session even with multi-write', function (done) { @@ -399,12 +399,15 @@ describe('session()', function(){ } request(server) - .get('/') - .expect(200, 'hello, world', function (err) { - if (err) return done(err) - assert.ok(saved) - done() - }) + .get('/') + .expect(200, 'hello, world', function (err) { + if (err) { + console.error('got err', { err }); + return done(err) + } + assert.ok(saved) + done() + }) }) it('should have saved session even with non-chunked response', function (done) { @@ -560,6 +563,7 @@ describe('session()', function(){ .get('/') .expect(shouldSetCookie('sessid')) .expect(200, 'session created', function (err, res) { + console.log('inside done'); if (err) return done(err) var val = cookie(res).replace(/...\./, '.') From 7f70f947d3d3183c6f9605537e1412e2b8724930 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Fri, 22 May 2020 17:22:30 +0200 Subject: [PATCH 04/23] it works --- index.js | 32 +++++++++++----------- test/session.js | 62 +++++++++++++++++++------------------------ test/support/utils.js | 12 +++++++++ 3 files changed, 56 insertions(+), 50 deletions(-) diff --git a/index.js b/index.js index f8452a5e..6af64aaf 100644 --- a/index.js +++ b/index.js @@ -247,7 +247,7 @@ function session(options) { var _end = res.end; var _write = res.write; var ended = false; - res.end = function end(c, e) { + res.end = function end(chunk, encoding, callback) { if (ended) { return false; } @@ -257,29 +257,29 @@ function session(options) { var ret; var sync = true; - var endArguments = Array.from(arguments); + var endArguments = [chunk, encoding, callback]; // var endArguments = arguments; // var chunk = endArguments[0]; // var encoding = endArguments[1]; - var chunk = endArguments[0]; - var encoding = endArguments[1]; console.log('about to shuffle arguments', { chunk: chunk, encoding: encoding }); - // // Callback may be the 1st (and only), second, or third argument - // if (typeof chunk === 'function') { - // console.log('chunk was function'); - // chunk = null; - // encoding = null; - // } else if (typeof encoding === 'function') { - // console.log('encoding was function'); - // encoding = null; - // } + // Callback may be the 1st (and only), second, or third argument + if (typeof chunk === 'function') { + console.log('chunk was function'); + callback = chunk; + chunk = null; + encoding = null; + } else if (typeof encoding === 'function') { + console.log('encoding was function'); + callback = encoding; + encoding = null; + } function writeend() { console.log('writeend') if (sync) { - ret = _end.apply(res, endArguments); + ret = _end.call(res, chunk, encoding, callback); sync = false; return; } @@ -340,7 +340,7 @@ function session(options) { if (!req.session) { console.log('has no session'); debug('no session'); - return _end.apply(res, endArguments); + return _end.call(res, chunk, encoding, callback); } if (!touched) { @@ -376,7 +376,7 @@ function session(options) { return writetop(); } - return _end.apply(res, endArguments); + return _end.call(res, chunk, encoding, callback); }; // generate the session diff --git a/test/session.js b/test/session.js index 66b73b5d..c7ca548c 100644 --- a/test/session.js +++ b/test/session.js @@ -189,39 +189,33 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - // it('should handle res.end(callback) calls', function (done) { - // var callbackHasBeenCalled = false; - // - // var server = createServer(null, function (req, res) { - // function callback() { - // callbackHasBeenCalled = true; - // } - // - // res.end(callback); - // }); - // - // request(server).get('/').expect(200, '', function () { - // var nodeVersionStrings = process.versions.node.split('.'); - // var nodeVersion = { - // major: Number(nodeVersionStrings[0]), - // minor: Number(nodeVersionStrings[1]), - // patch: Number(nodeVersionStrings[2]) - // } - // - // console.log('nodejs version', {nodeVersion: nodeVersion }); - // - // if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // // Node versions prior to 0.11.6 do not support the callback argument - // console.log('callback should NOT be called') - // assert.ok(!callbackHasBeenCalled) - // } else { - // console.log('callback SHOULD be called') - // assert.ok(callbackHasBeenCalled) - // } - // - // done() - // }) - // }); + it.only('should handle res.end(callback) calls', function (done) { + var callbackHasBeenCalled = false; + + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } + + res.end(callback); + }); + + request(server).get('/').expect(200, '', function () { + const nodeVersion = utils.getNodeVersion(); + + if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // Node versions prior to 0.11.6 do not support the callback argument, + // so it should not have been called. + console.log('callback should NOT be called') + assert.ok(!callbackHasBeenCalled) + } else { + console.log('callback SHOULD be called') + assert.ok(callbackHasBeenCalled) + } + + done() + }) + }); it('should handle res.end(null) calls', function (done) { var server = createServer(null, function (req, res) { @@ -378,7 +372,7 @@ describe('session()', function(){ }) }) - it.only('should have saved session even with multi-write', function (done) { + it('should have saved session even with multi-write', function (done) { var saved = false var store = new session.MemoryStore() var server = createServer({ store: store }, function (req, res) { diff --git a/test/support/utils.js b/test/support/utils.js index 0f13ba6a..ea56da35 100644 --- a/test/support/utils.js +++ b/test/support/utils.js @@ -2,6 +2,7 @@ module.exports.parseSetCookie = parseSetCookie module.exports.writePatch = writePatch +module.exports.getNodeVersion = getNodeVersion function parseSetCookie (header) { var match @@ -40,3 +41,14 @@ function writePatch (res) { return _write.apply(this, arguments) } } + +function getNodeVersion () { + var nodeVersionStrings = process.versions.node.split('.'); + var nodeVersion = { + major: Number(nodeVersionStrings[0]), + minor: Number(nodeVersionStrings[1]), + patch: Number(nodeVersionStrings[2]) + } + + return nodeVersion; +} From 2c8a4ed8ecd7e5f7cd6f5a9638f61946f266e74b Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 20:09:16 +0200 Subject: [PATCH 05/23] clean up --- test/session.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/session.js b/test/session.js index c7ca548c..5c55f5bb 100644 --- a/test/session.js +++ b/test/session.js @@ -189,7 +189,7 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - it.only('should handle res.end(callback) calls', function (done) { + it('should handle res.end(callback) calls', function (done) { var callbackHasBeenCalled = false; var server = createServer(null, function (req, res) { @@ -395,10 +395,7 @@ describe('session()', function(){ request(server) .get('/') .expect(200, 'hello, world', function (err) { - if (err) { - console.error('got err', { err }); - return done(err) - } + if (err) return done(err) assert.ok(saved) done() }) From 38327907cbc0c53f5cf78e3e389826f34298ce2a Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 20:10:05 +0200 Subject: [PATCH 06/23] remove console.log statements --- test/session.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/session.js b/test/session.js index 5c55f5bb..ea28a7ff 100644 --- a/test/session.js +++ b/test/session.js @@ -206,10 +206,8 @@ describe('session()', function(){ if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { // Node versions prior to 0.11.6 do not support the callback argument, // so it should not have been called. - console.log('callback should NOT be called') assert.ok(!callbackHasBeenCalled) } else { - console.log('callback SHOULD be called') assert.ok(callbackHasBeenCalled) } @@ -554,7 +552,6 @@ describe('session()', function(){ .get('/') .expect(shouldSetCookie('sessid')) .expect(200, 'session created', function (err, res) { - console.log('inside done'); if (err) return done(err) var val = cookie(res).replace(/...\./, '.') From e909d4a186bfd5f22a3fca6ff94c5691e8b1c4df Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 20:12:04 +0200 Subject: [PATCH 07/23] remove more debug stuff --- index.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/index.js b/index.js index 6af64aaf..536114e3 100644 --- a/index.js +++ b/index.js @@ -257,27 +257,17 @@ function session(options) { var ret; var sync = true; - var endArguments = [chunk, encoding, callback]; - // var endArguments = arguments; - // var chunk = endArguments[0]; - // var encoding = endArguments[1]; - - console.log('about to shuffle arguments', { chunk: chunk, encoding: encoding }); - // Callback may be the 1st (and only), second, or third argument if (typeof chunk === 'function') { - console.log('chunk was function'); callback = chunk; chunk = null; encoding = null; } else if (typeof encoding === 'function') { - console.log('encoding was function'); callback = encoding; encoding = null; } function writeend() { - console.log('writeend') if (sync) { ret = _end.call(res, chunk, encoding, callback); sync = false; @@ -321,7 +311,6 @@ function session(options) { } if (shouldDestroy(req)) { - console.log('shouldDestroy'); // destroy session debug('destroying'); store.destroy(req.sessionID, function ondestroy(err) { @@ -338,20 +327,17 @@ function session(options) { // no session to save if (!req.session) { - console.log('has no session'); debug('no session'); return _end.call(res, chunk, encoding, callback); } if (!touched) { - console.log('session was not touched') // touch session req.session.touch() touched = true } if (shouldSave(req)) { - console.log('should save') req.session.save(function onsave(err) { if (err) { defer(next, err); From 655b36f63c64e1d5680538113a2a31846002f719 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 20:21:17 +0200 Subject: [PATCH 08/23] minimize diff --- .gitignore | 1 - test/session.js | 36 ++++++++++++++++++------------------ 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/.gitignore b/.gitignore index 615e06d4..207febba 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,3 @@ -.idea/ .nyc_output coverage node_modules diff --git a/test/session.js b/test/session.js index ea28a7ff..a87a590c 100644 --- a/test/session.js +++ b/test/session.js @@ -334,12 +334,12 @@ describe('session()', function(){ } request(server) - .get('/') - .expect(200, 'session saved', function (err) { - if (err) return done(err) - assert.ok(saved) - done() - }) + .get('/') + .expect(200, 'session saved', function (err) { + if (err) return done(err) + assert.ok(saved) + done() + }) }) it('should have saved session even with empty response', function (done) { @@ -362,12 +362,12 @@ describe('session()', function(){ } request(server) - .get('/') - .expect(200, '', function (err) { - if (err) return done(err) - assert.ok(saved) - done() - }) + .get('/') + .expect(200, '', function (err) { + if (err) return done(err) + assert.ok(saved) + done() + }) }) it('should have saved session even with multi-write', function (done) { @@ -391,12 +391,12 @@ describe('session()', function(){ } request(server) - .get('/') - .expect(200, 'hello, world', function (err) { - if (err) return done(err) - assert.ok(saved) - done() - }) + .get('/') + .expect(200, 'hello, world', function (err) { + if (err) return done(err) + assert.ok(saved) + done() + }) }) it('should have saved session even with non-chunked response', function (done) { From afa311d728223de2f4af1f09c1e1e2ab9d51ae3a Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 20:24:35 +0200 Subject: [PATCH 09/23] revert package.json changes --- package.json | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index f0bea203..ba2adb65 100644 --- a/package.json +++ b/package.json @@ -22,9 +22,12 @@ "devDependencies": { "after": "0.8.2", "cookie-parser": "1.4.5", + "eslint": "3.19.0", + "eslint-plugin-markdown": "1.0.2", "express": "4.17.1", - "mocha": "2.5.3", - "supertest": "1.1.0" + "mocha": "7.1.1", + "nyc": "15.0.1", + "supertest": "4.0.2" }, "files": [ "session/", From b6bce2debe96088786db34ef2ad83352bcabfd11 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 20:26:52 +0200 Subject: [PATCH 10/23] simplify node version test util --- test/support/utils.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/support/utils.js b/test/support/utils.js index ea56da35..a0ace1ff 100644 --- a/test/support/utils.js +++ b/test/support/utils.js @@ -43,12 +43,11 @@ function writePatch (res) { } function getNodeVersion () { - var nodeVersionStrings = process.versions.node.split('.'); - var nodeVersion = { - major: Number(nodeVersionStrings[0]), - minor: Number(nodeVersionStrings[1]), - patch: Number(nodeVersionStrings[2]) - } + var nodeVersionNumbers = process.versions.node.split('.').map(Number); - return nodeVersion; + return { + major: nodeVersionNumbers[0], + minor: nodeVersionNumbers[1], + patch: nodeVersionNumbers[2] + } } From 76e3c106e802007ccb22811c0883629e20b144ad Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 21:31:48 +0200 Subject: [PATCH 11/23] test all cases & make it work when passing (data, callback) --- index.js | 2 +- test/session.js | 54 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 536114e3..d120ff9d 100644 --- a/index.js +++ b/index.js @@ -274,7 +274,7 @@ function session(options) { return; } - _end.call(res); + _end.call(res, callback); } function writetop() { diff --git a/test/session.js b/test/session.js index a87a590c..27850f69 100644 --- a/test/session.js +++ b/test/session.js @@ -201,7 +201,59 @@ describe('session()', function(){ }); request(server).get('/').expect(200, '', function () { - const nodeVersion = utils.getNodeVersion(); + var nodeVersion = utils.getNodeVersion(); + + if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // Node versions prior to 0.11.6 do not support the callback argument, + // so it should not have been called. + assert.ok(!callbackHasBeenCalled) + } else { + assert.ok(callbackHasBeenCalled) + } + + done() + }) + }); + + it('should handle res.end(data, callback) calls', function (done) { + var callbackHasBeenCalled = false; + + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } + + res.end('hello', callback); + }); + + request(server).get('/').expect(200, 'hello', function () { + var nodeVersion = utils.getNodeVersion(); + + if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // Node versions prior to 0.11.6 do not support the callback argument, + // so it should not have been called. + assert.ok(!callbackHasBeenCalled) + } else { + assert.ok(callbackHasBeenCalled) + } + + done() + }) + }); + + it('should handle res.end(data, encoding, callback) calls', function (done) { + var callbackHasBeenCalled = false; + + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } + + res.end('hello', 'utf8', callback); + }); + + request(server).get('/').expect(200, 'hello', function () { + var nodeVersion = utils.getNodeVersion(); if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { // Node versions prior to 0.11.6 do not support the callback argument, From 3591378df335a990efc5aba384b545c51aa51df1 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 21:35:45 +0200 Subject: [PATCH 12/23] pull tests into describe block --- test/session.js | 120 ++++++++++++++++++++++++------------------------ 1 file changed, 61 insertions(+), 59 deletions(-) diff --git a/test/session.js b/test/session.js index 27850f69..b7695b7a 100644 --- a/test/session.js +++ b/test/session.js @@ -189,82 +189,84 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - it('should handle res.end(callback) calls', function (done) { - var callbackHasBeenCalled = false; + describe('res.end() proxy', function () { + it('should correctly handle callback as only argument', function (done) { + var callbackHasBeenCalled = false; - var server = createServer(null, function (req, res) { - function callback() { - callbackHasBeenCalled = true; - } + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } - res.end(callback); - }); + res.end(callback); + }); - request(server).get('/').expect(200, '', function () { - var nodeVersion = utils.getNodeVersion(); + request(server).get('/').expect(200, '', function () { + var nodeVersion = utils.getNodeVersion(); - if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // Node versions prior to 0.11.6 do not support the callback argument, - // so it should not have been called. - assert.ok(!callbackHasBeenCalled) - } else { - assert.ok(callbackHasBeenCalled) - } + if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // Node versions prior to 0.11.6 do not support the callback argument, + // so it should not have been called. + assert.ok(!callbackHasBeenCalled) + } else { + assert.ok(callbackHasBeenCalled) + } - done() - }) - }); + done() + }) + }); - it('should handle res.end(data, callback) calls', function (done) { - var callbackHasBeenCalled = false; + it('should correctly handle callback as second argument', function (done) { + var callbackHasBeenCalled = false; - var server = createServer(null, function (req, res) { - function callback() { - callbackHasBeenCalled = true; - } + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } - res.end('hello', callback); - }); + res.end('hello', callback); + }); - request(server).get('/').expect(200, 'hello', function () { - var nodeVersion = utils.getNodeVersion(); + request(server).get('/').expect(200, 'hello', function () { + var nodeVersion = utils.getNodeVersion(); - if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // Node versions prior to 0.11.6 do not support the callback argument, - // so it should not have been called. - assert.ok(!callbackHasBeenCalled) - } else { - assert.ok(callbackHasBeenCalled) - } + if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // Node versions prior to 0.11.6 do not support the callback argument, + // so it should not have been called. + assert.ok(!callbackHasBeenCalled) + } else { + assert.ok(callbackHasBeenCalled) + } - done() - }) - }); + done() + }) + }); - it('should handle res.end(data, encoding, callback) calls', function (done) { - var callbackHasBeenCalled = false; + it('should correctly handle callback as third argument', function (done) { + var callbackHasBeenCalled = false; - var server = createServer(null, function (req, res) { - function callback() { - callbackHasBeenCalled = true; - } + var server = createServer(null, function (req, res) { + function callback() { + callbackHasBeenCalled = true; + } - res.end('hello', 'utf8', callback); - }); + res.end('hello', 'utf8', callback); + }); - request(server).get('/').expect(200, 'hello', function () { - var nodeVersion = utils.getNodeVersion(); + request(server).get('/').expect(200, 'hello', function () { + var nodeVersion = utils.getNodeVersion(); - if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // Node versions prior to 0.11.6 do not support the callback argument, - // so it should not have been called. - assert.ok(!callbackHasBeenCalled) - } else { - assert.ok(callbackHasBeenCalled) - } + if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { + // Node versions prior to 0.11.6 do not support the callback argument, + // so it should not have been called. + assert.ok(!callbackHasBeenCalled) + } else { + assert.ok(callbackHasBeenCalled) + } - done() - }) + done() + }) + }); }); it('should handle res.end(null) calls', function (done) { From 0a5d51a571073aafb29140a83b267c4ef5f09b5e Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 21:41:06 +0200 Subject: [PATCH 13/23] extract node version support stuff to variable --- test/session.js | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/test/session.js b/test/session.js index b7695b7a..560ecf48 100644 --- a/test/session.js +++ b/test/session.js @@ -190,6 +190,12 @@ describe('session()', function(){ }) describe('res.end() proxy', function () { + var nodeVersion = utils.getNodeVersion(); + + // Node versions prior to 0.11.6 do not support the callback argument, + // so the callback should not have been called. + var nodeVersionSupportsResEndCallback = nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6)); + it('should correctly handle callback as only argument', function (done) { var callbackHasBeenCalled = false; @@ -202,11 +208,7 @@ describe('session()', function(){ }); request(server).get('/').expect(200, '', function () { - var nodeVersion = utils.getNodeVersion(); - - if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // Node versions prior to 0.11.6 do not support the callback argument, - // so it should not have been called. + if (nodeVersionSupportsResEndCallback) { assert.ok(!callbackHasBeenCalled) } else { assert.ok(callbackHasBeenCalled) @@ -228,11 +230,7 @@ describe('session()', function(){ }); request(server).get('/').expect(200, 'hello', function () { - var nodeVersion = utils.getNodeVersion(); - - if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // Node versions prior to 0.11.6 do not support the callback argument, - // so it should not have been called. + if (nodeVersionSupportsResEndCallback) { assert.ok(!callbackHasBeenCalled) } else { assert.ok(callbackHasBeenCalled) @@ -256,9 +254,7 @@ describe('session()', function(){ request(server).get('/').expect(200, 'hello', function () { var nodeVersion = utils.getNodeVersion(); - if (nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6))) { - // Node versions prior to 0.11.6 do not support the callback argument, - // so it should not have been called. + if (nodeVersionSupportsResEndCallback) { assert.ok(!callbackHasBeenCalled) } else { assert.ok(callbackHasBeenCalled) From f93f99565acc4bb89c9b6ea6e2ed5aabadac5815 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 21:51:30 +0200 Subject: [PATCH 14/23] switch from call to apply --- index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index d120ff9d..53e1269a 100644 --- a/index.js +++ b/index.js @@ -269,12 +269,12 @@ function session(options) { function writeend() { if (sync) { - ret = _end.call(res, chunk, encoding, callback); + ret = _end.apply(res, [chunk, encoding, callback]); sync = false; return; } - _end.call(res, callback); + _end.apply(res, [callback]); } function writetop() { @@ -328,7 +328,7 @@ function session(options) { // no session to save if (!req.session) { debug('no session'); - return _end.call(res, chunk, encoding, callback); + return _end.apply(res, [chunk, encoding, callback]); } if (!touched) { @@ -362,7 +362,7 @@ function session(options) { return writetop(); } - return _end.call(res, chunk, encoding, callback); + return _end.apply(res, [chunk, encoding, callback]); }; // generate the session From 6e88688bcc5b4d25b5142a74f6a295efeaaac0cb Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Tue, 26 May 2020 22:06:16 +0200 Subject: [PATCH 15/23] mutate arguments to Make It Work --- index.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 53e1269a..3d5aed84 100644 --- a/index.js +++ b/index.js @@ -257,6 +257,8 @@ function session(options) { var ret; var sync = true; + var endArgs = arguments; + // Callback may be the 1st (and only), second, or third argument if (typeof chunk === 'function') { callback = chunk; @@ -269,7 +271,7 @@ function session(options) { function writeend() { if (sync) { - ret = _end.apply(res, [chunk, encoding, callback]); + ret = _end.apply(res, endArgs); sync = false; return; } @@ -294,12 +296,15 @@ function session(options) { chunk = !Buffer.isBuffer(chunk) ? Buffer.from(chunk, encoding) : chunk; + endArgs[0] = chunk; encoding = undefined; + endArgs[1] = encoding; if (chunk.length !== 0) { debug('split response'); ret = _write.call(res, chunk.slice(0, chunk.length - 1)); chunk = chunk.slice(chunk.length - 1, chunk.length); + endArgs[0] = chunk; return ret; } } @@ -328,7 +333,7 @@ function session(options) { // no session to save if (!req.session) { debug('no session'); - return _end.apply(res, [chunk, encoding, callback]); + return _end.apply(res, endArgs); } if (!touched) { @@ -362,7 +367,7 @@ function session(options) { return writetop(); } - return _end.apply(res, [chunk, encoding, callback]); + return _end.apply(res, endArgs); }; // generate the session From 647edce5f952d3c3426f17b524e2c33555078a54 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Wed, 27 May 2020 14:45:01 +0200 Subject: [PATCH 16/23] update --- test/session.js | 81 ++++++++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/test/session.js b/test/session.js index 560ecf48..6e7bef47 100644 --- a/test/session.js +++ b/test/session.js @@ -189,81 +189,91 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - describe('res.end() proxy', function () { + describe.only('res.end() proxy', function () { var nodeVersion = utils.getNodeVersion(); - // Node versions prior to 0.11.6 do not support the callback argument, - // so the callback should not have been called. - var nodeVersionSupportsResEndCallback = nodeVersion.major === 0 && (nodeVersion.minor < 11 || (nodeVersion.minor === 11 && nodeVersion.patch < 6)); + // Node versions prior to 0.11.6 do not explicitly support the callback argument. + // These versions, if given a callback as the… + // - first argument, throw a TypeError + // - second argument, execute the callback (by accident) + // - third argument, ignore the callback + var nodeVersionSupportsResEndCallback = nodeVersion.major > 0 || nodeVersion.minor > 11 || (nodeVersion.minor === 11 && nodeVersion.patch >= 6); it('should correctly handle callback as only argument', function (done) { - var callbackHasBeenCalled = false; + var callbackHasBeenCalled = false var server = createServer(null, function (req, res) { function callback() { - callbackHasBeenCalled = true; + callbackHasBeenCalled = true } - res.end(callback); + try { + res.end(callback) + } catch (err) { + // The res#end proxy passes on the arguments exactly as provided, + // so we expect a TypeError in versions that don't support the callback. + // This is a way of notifying the test client that an error occurred. + res.writeHead(500) + res.end(err.message) + } }); - request(server).get('/').expect(200, '', function () { - if (nodeVersionSupportsResEndCallback) { - assert.ok(!callbackHasBeenCalled) - } else { - assert.ok(callbackHasBeenCalled) - } + var call = request(server).get('/'); - done() - }) + if (nodeVersionSupportsResEndCallback) { + call.expect(200, '', function () { + assert.ok(callbackHasBeenCalled) + done() + }) + } else { + call.expect(500, 'first argument must be a string or Buffer', done) + } }); it('should correctly handle callback as second argument', function (done) { - var callbackHasBeenCalled = false; + var callbackHasBeenCalled = false var server = createServer(null, function (req, res) { function callback() { - callbackHasBeenCalled = true; + callbackHasBeenCalled = true } - res.end('hello', callback); + res.end('hello', callback) }); request(server).get('/').expect(200, 'hello', function () { - if (nodeVersionSupportsResEndCallback) { - assert.ok(!callbackHasBeenCalled) - } else { - assert.ok(callbackHasBeenCalled) - } - + // Passing the callback as the second argument works on all versions of Node, + // seemingly by accident. The encoding parameter is passed to afterWrite, + // which supports that it might be a callback. + assert.ok(callbackHasBeenCalled) done() }) - }); + }) it('should correctly handle callback as third argument', function (done) { - var callbackHasBeenCalled = false; + var callbackHasBeenCalled = false var server = createServer(null, function (req, res) { function callback() { - callbackHasBeenCalled = true; + callbackHasBeenCalled = true } - res.end('hello', 'utf8', callback); + res.end('hello', 'utf8', callback) }); request(server).get('/').expect(200, 'hello', function () { - var nodeVersion = utils.getNodeVersion(); - + // The third argument is completely ignored on Node versions + // that do not support the callback. if (nodeVersionSupportsResEndCallback) { - assert.ok(!callbackHasBeenCalled) - } else { assert.ok(callbackHasBeenCalled) + } else { + assert.ok(!callbackHasBeenCalled) } done() }) - }); - }); + }) + }) it('should handle res.end(null) calls', function (done) { var server = createServer(null, function (req, res) { @@ -2367,8 +2377,9 @@ function createServer (options, respond) { } function createRequestListener(opts, fn) { - var _session = createSession(opts) + // var _session = createSession(opts) var respond = fn || end + var _session = respond; return function onRequest(req, res) { var server = this From 94a85cd3bd9206affe64c1fce29bccfe6737130c Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Wed, 27 May 2020 15:13:50 +0200 Subject: [PATCH 17/23] what --- index.js | 32 +++++++++++++++++++++++--------- test/session.js | 9 ++++----- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index 3d5aed84..1c578a07 100644 --- a/index.js +++ b/index.js @@ -259,14 +259,26 @@ function session(options) { var endArgs = arguments; - // Callback may be the 1st (and only), second, or third argument - if (typeof chunk === 'function') { - callback = chunk; - chunk = null; - encoding = null; - } else if (typeof encoding === 'function') { - callback = encoding; - encoding = null; + var callbackArgumentIndex = typeof chunk === 'function' + ? 0 + : typeof encoding === 'function' + ? 1 + : 2; + + // Callback may be the first, second, or third argument. + // If provided, it must be the last argument. + switch (callbackArgumentIndex) { + case 0: { + callback = chunk; + chunk = null; + encoding = null; + break; + } + case 1: { + callback = encoding; + encoding = null; + break; + } } function writeend() { @@ -276,7 +288,9 @@ function session(options) { return; } - _end.apply(res, [callback]); + var argumentsWithoutChunkOrEncoding = [null, null, null]; + argumentsWithoutChunkOrEncoding[callbackArgumentIndex] = callback; + _end.apply(res, argumentsWithoutChunkOrEncoding); } function writetop() { diff --git a/test/session.js b/test/session.js index 6e7bef47..43b7a3c5 100644 --- a/test/session.js +++ b/test/session.js @@ -189,7 +189,7 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - describe.only('res.end() proxy', function () { + describe('res.end() proxy', function () { var nodeVersion = utils.getNodeVersion(); // Node versions prior to 0.11.6 do not explicitly support the callback argument. @@ -235,6 +235,7 @@ describe('session()', function(){ var server = createServer(null, function (req, res) { function callback() { + console.log('what'); callbackHasBeenCalled = true } @@ -243,8 +244,7 @@ describe('session()', function(){ request(server).get('/').expect(200, 'hello', function () { // Passing the callback as the second argument works on all versions of Node, - // seemingly by accident. The encoding parameter is passed to afterWrite, - // which supports that it might be a callback. + // seemingly by accident. assert.ok(callbackHasBeenCalled) done() }) @@ -2377,9 +2377,8 @@ function createServer (options, respond) { } function createRequestListener(opts, fn) { - // var _session = createSession(opts) + var _session = createSession(opts) var respond = fn || end - var _session = respond; return function onRequest(req, res) { var server = this From c9abb7e3b960b680ab5c8f4ddc07d6839ad5d4a6 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Wed, 27 May 2020 15:18:26 +0200 Subject: [PATCH 18/23] wat --- index.js | 19 +++++++------------ test/session.js | 2 +- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/index.js b/index.js index 1c578a07..87348a4f 100644 --- a/index.js +++ b/index.js @@ -267,18 +267,13 @@ function session(options) { // Callback may be the first, second, or third argument. // If provided, it must be the last argument. - switch (callbackArgumentIndex) { - case 0: { - callback = chunk; - chunk = null; - encoding = null; - break; - } - case 1: { - callback = encoding; - encoding = null; - break; - } + if (callbackArgumentIndex === 0) { + callback = chunk; + chunk = null; + encoding = null; + } else if (callbackArgumentIndex === 1) { + callback = encoding; + encoding = null; } function writeend() { diff --git a/test/session.js b/test/session.js index 43b7a3c5..f9505188 100644 --- a/test/session.js +++ b/test/session.js @@ -189,7 +189,7 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - describe('res.end() proxy', function () { + describe.only('res.end() proxy', function () { var nodeVersion = utils.getNodeVersion(); // Node versions prior to 0.11.6 do not explicitly support the callback argument. From 5fce3e3dac1e23697d42f937a8663de59c0c8d6f Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Wed, 27 May 2020 17:53:33 +0200 Subject: [PATCH 19/23] still WIP --- index.js | 21 ++++++++++++++++++--- test/session.js | 50 +++++++++++++++++++++++++------------------------ 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/index.js b/index.js index 87348a4f..13ece8cc 100644 --- a/index.js +++ b/index.js @@ -265,6 +265,7 @@ function session(options) { ? 1 : 2; + debug('callbackArgumentIndex', callbackArgumentIndex) // Callback may be the first, second, or third argument. // If provided, it must be the last argument. if (callbackArgumentIndex === 0) { @@ -277,6 +278,7 @@ function session(options) { } function writeend() { + debug('writeend', { sync: sync }) if (sync) { ret = _end.apply(res, endArgs); sync = false; @@ -285,6 +287,7 @@ function session(options) { var argumentsWithoutChunkOrEncoding = [null, null, null]; argumentsWithoutChunkOrEncoding[callbackArgumentIndex] = callback; + debug('applying _end with', { argumentsWithoutChunkOrEncoding: argumentsWithoutChunkOrEncoding }) _end.apply(res, argumentsWithoutChunkOrEncoding); } @@ -333,7 +336,11 @@ function session(options) { } debug('destroyed'); - writeend(); + try { + writeend(); + } catch (err) { + next(err); + } }); return writetop(); @@ -357,7 +364,11 @@ function session(options) { defer(next, err); } - writeend(); + try { + writeend(); + } catch (err) { + next(err); + } }); return writetop(); @@ -370,7 +381,11 @@ function session(options) { } debug('touched'); - writeend(); + try { + writeend(); + } catch (err) { + next(err); + } }); return writetop(); diff --git a/test/session.js b/test/session.js index f9505188..c27aa72d 100644 --- a/test/session.js +++ b/test/session.js @@ -189,7 +189,7 @@ describe('session()', function(){ .expect(200, 'Hello, world!', done); }) - describe.only('res.end() proxy', function () { + describe('res.end() proxy', function () { var nodeVersion = utils.getNodeVersion(); // Node versions prior to 0.11.6 do not explicitly support the callback argument. @@ -207,30 +207,24 @@ describe('session()', function(){ callbackHasBeenCalled = true } - try { - res.end(callback) - } catch (err) { - // The res#end proxy passes on the arguments exactly as provided, - // so we expect a TypeError in versions that don't support the callback. - // This is a way of notifying the test client that an error occurred. - res.writeHead(500) - res.end(err.message) - } - }); - - var call = request(server).get('/'); - - if (nodeVersionSupportsResEndCallback) { - call.expect(200, '', function () { - assert.ok(callbackHasBeenCalled) + server.once('error', function onerror (err) { + assert.ok(err) + assert.strictEqual(err.message, 'first argument must be a string or Buffer') done() }) - } else { - call.expect(500, 'first argument must be a string or Buffer', done) - } + + res.end(callback) + }); + + request(server) + .get('/') + .expect(200, '', function () { + assert.ok(callbackHasBeenCalled) + done() + }) }); - it('should correctly handle callback as second argument', function (done) { + it.only('should correctly handle callback as second argument', function (done) { var callbackHasBeenCalled = false var server = createServer(null, function (req, res) { @@ -239,11 +233,17 @@ describe('session()', function(){ callbackHasBeenCalled = true } + console.log('ending'); res.end('hello', callback) }); - request(server).get('/').expect(200, 'hello', function () { - // Passing the callback as the second argument works on all versions of Node, + request(server).get('/').expect(200, 'hello', function (err, response) { + if (err) { + console.log('err', err); + } + console.log('response', response.text); + + // Passing the callback as the second argument works on all Node versions, // seemingly by accident. assert.ok(callbackHasBeenCalled) done() @@ -267,7 +267,7 @@ describe('session()', function(){ if (nodeVersionSupportsResEndCallback) { assert.ok(callbackHasBeenCalled) } else { - assert.ok(!callbackHasBeenCalled) + assert.strictEqual(callbackHasBeenCalled, false) } done() @@ -2384,6 +2384,7 @@ function createRequestListener(opts, fn) { var server = this _session(req, res, function (err) { + console.log('err inside _session next', err) if (err && !res._header) { res.statusCode = err.status || 500 res.end(err.message) @@ -2395,6 +2396,7 @@ function createRequestListener(opts, fn) { return } + console.log('calling respond') respond(req, res) }) } From d4856f82efcf3acc4dfcf9d96945a1c89074f860 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Thu, 28 May 2020 13:39:55 +0200 Subject: [PATCH 20/23] clean up --- index.js | 14 +++++++++----- test/session.js | 28 ++++++++++++---------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/index.js b/index.js index 13ece8cc..14e777e5 100644 --- a/index.js +++ b/index.js @@ -257,17 +257,18 @@ function session(options) { var ret; var sync = true; + // Keep a reference to the original arguments so we can preserve their + // order when passing them on to the original _end() var endArgs = arguments; + // The callback argument is optional. If provided, it may be the + // first, second, or third argument, and must be the last argument. var callbackArgumentIndex = typeof chunk === 'function' ? 0 : typeof encoding === 'function' ? 1 : 2; - debug('callbackArgumentIndex', callbackArgumentIndex) - // Callback may be the first, second, or third argument. - // If provided, it must be the last argument. if (callbackArgumentIndex === 0) { callback = chunk; chunk = null; @@ -278,16 +279,19 @@ function session(options) { } function writeend() { - debug('writeend', { sync: sync }) if (sync) { ret = _end.apply(res, endArgs); sync = false; return; } + // This differs very slightly from (undocumented) Node behaviour in versions + // prior to 0.11.6 (which introduced callback support), because we explicitly + // call write() followed by end(). + // The behaviour seems unintentional, as the callback is invoked immediately + // after write(), as opposed to after end(). var argumentsWithoutChunkOrEncoding = [null, null, null]; argumentsWithoutChunkOrEncoding[callbackArgumentIndex] = callback; - debug('applying _end with', { argumentsWithoutChunkOrEncoding: argumentsWithoutChunkOrEncoding }) _end.apply(res, argumentsWithoutChunkOrEncoding); } diff --git a/test/session.js b/test/session.js index c27aa72d..1d1d6224 100644 --- a/test/session.js +++ b/test/session.js @@ -192,11 +192,7 @@ describe('session()', function(){ describe('res.end() proxy', function () { var nodeVersion = utils.getNodeVersion(); - // Node versions prior to 0.11.6 do not explicitly support the callback argument. - // These versions, if given a callback as the… - // - first argument, throw a TypeError - // - second argument, execute the callback (by accident) - // - third argument, ignore the callback + // Support for the callback argument was added in Node 0.11.6. var nodeVersionSupportsResEndCallback = nodeVersion.major > 0 || nodeVersion.minor > 11 || (nodeVersion.minor === 11 && nodeVersion.patch >= 6); it('should correctly handle callback as only argument', function (done) { @@ -229,23 +225,25 @@ describe('session()', function(){ var server = createServer(null, function (req, res) { function callback() { - console.log('what'); callbackHasBeenCalled = true } - console.log('ending'); res.end('hello', callback) }); - request(server).get('/').expect(200, 'hello', function (err, response) { - if (err) { - console.log('err', err); + request(server).get('/').expect(200, 'hello', function () { + if (nodeVersionSupportsResEndCallback) { + assert.ok(callbackHasBeenCalled) + } else { + // This is (very slightly) different from the default Node behaviour, + // where the callback is invoked if `chunk` is a non-empty string or Buffer. + // This behaviour is undocumented, and seemingly happens unintentionally + // when the chunk and encoding are passed to write(), and in turn to + // afterWrite(), which considers the encoding argument to possibly be + // a function, and invokes it. + assert.strictEqual(callbackHasBeenCalled, false) } - console.log('response', response.text); - // Passing the callback as the second argument works on all Node versions, - // seemingly by accident. - assert.ok(callbackHasBeenCalled) done() }) }) @@ -2384,7 +2382,6 @@ function createRequestListener(opts, fn) { var server = this _session(req, res, function (err) { - console.log('err inside _session next', err) if (err && !res._header) { res.statusCode = err.status || 500 res.end(err.message) @@ -2396,7 +2393,6 @@ function createRequestListener(opts, fn) { return } - console.log('calling respond') respond(req, res) }) } From a5b85af36977f505b6dfb03b1ccd2cd319bdeaa1 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Thu, 28 May 2020 13:43:46 +0200 Subject: [PATCH 21/23] don't use .only --- test/session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/session.js b/test/session.js index 1d1d6224..26c69fc1 100644 --- a/test/session.js +++ b/test/session.js @@ -220,7 +220,7 @@ describe('session()', function(){ }) }); - it.only('should correctly handle callback as second argument', function (done) { + it('should correctly handle callback as second argument', function (done) { var callbackHasBeenCalled = false var server = createServer(null, function (req, res) { From 3a5df79c1892ac6fa8cbd04381c80299d0aa07ae Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Thu, 28 May 2020 13:57:37 +0200 Subject: [PATCH 22/23] don't overwrite second argument if it isn't encoding --- index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 14e777e5..12f94815 100644 --- a/index.js +++ b/index.js @@ -314,7 +314,9 @@ function session(options) { : chunk; endArgs[0] = chunk; encoding = undefined; - endArgs[1] = encoding; + if (callbackArgumentIndex !== 1) { + endArgs[1] = encoding; + } if (chunk.length !== 0) { debug('split response'); From a578c5010ebeefea7aa3b348cbad559d2cdb1f99 Mon Sep 17 00:00:00 2001 From: Martin Lehmann Date: Thu, 28 May 2020 14:29:54 +0200 Subject: [PATCH 23/23] don't assert or call done if not required --- test/session.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/session.js b/test/session.js index 26c69fc1..f7e4b01f 100644 --- a/test/session.js +++ b/test/session.js @@ -203,7 +203,7 @@ describe('session()', function(){ callbackHasBeenCalled = true } - server.once('error', function onerror (err) { + server.on('error', function onerror (err) { assert.ok(err) assert.strictEqual(err.message, 'first argument must be a string or Buffer') done() @@ -215,8 +215,12 @@ describe('session()', function(){ request(server) .get('/') .expect(200, '', function () { - assert.ok(callbackHasBeenCalled) - done() + if (nodeVersionSupportsResEndCallback) { + // If the Node version does not support the callback, + // the error listener will make the necessary assertions. + assert.ok(callbackHasBeenCalled) + done() + } }) });