From d32158eec6bdf67f604afca7572037e6c06aae61 Mon Sep 17 00:00:00 2001 From: Kevin Locke Date: Fri, 8 May 2015 15:52:45 -0600 Subject: [PATCH] Fix pause before response Due to #1568, we now propagate pause and resume to the response content stream, rather than the response stream. However, when pause is called before the response arrives, the pause is still being applied to the response object directly. Fix this by applying it to the response content stream in both cases. This avoids the issue that if pause is called before the response arrives, it can not be resumed. Also add tests of the pause/resume behavior for both the gzip and non-gzip case both before and after the response has arrived. This commit also makes the ancillary change that resume is now called unconditionally (when defined) in Redirect, since we always want to dump the response data. Signed-off-by: Kevin Locke --- lib/redirect.js | 3 +- request.js | 10 +++---- tests/test-gzip.js | 73 +++++++++++++++++++++++++++++++++++++++++++-- tests/test-pipes.js | 60 +++++++++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 9 deletions(-) diff --git a/lib/redirect.js b/lib/redirect.js index 7dd6c254c..ce83ffec8 100644 --- a/lib/redirect.js +++ b/lib/redirect.js @@ -87,7 +87,8 @@ Redirect.prototype.onResponse = function (response) { // ignore any potential response body. it cannot possibly be useful // to us at this point. - if (request._paused) { + // response.resume should be defined, but check anyway before calling. Workaround for browserify. + if (response.resume) { response.resume() } diff --git a/request.js b/request.js index f8c33f49f..88cf6dd5d 100644 --- a/request.js +++ b/request.js @@ -1047,12 +1047,6 @@ Request.prototype.onRequestResponse = function (response) { response.resume() return } - if (self._paused) { - response.pause() - } else if (response.resume) { - // response.resume should be defined, but check anyway before calling. Workaround for browserify. - response.resume() - } self.response = response response.request = self @@ -1153,6 +1147,10 @@ Request.prototype.onRequestResponse = function (response) { } } + if (self._paused) { + responseContent.pause() + } + self.responseContent = responseContent self.emit('response', response) diff --git a/tests/test-gzip.js b/tests/test-gzip.js index 5c2b3c1ae..35949dd1c 100644 --- a/tests/test-gzip.js +++ b/tests/test-gzip.js @@ -7,6 +7,8 @@ var request = require('../index') , tape = require('tape') var testContent = 'Compressible response content.\n' + , testContentBig + , testContentBigGzip , testContentGzip var server = http.createServer(function(req, res) { @@ -18,6 +20,10 @@ var server = http.createServer(function(req, res) { if (req.url === '/error') { // send plaintext instead of gzip (should cause an error for the client) res.end(testContent) + } else if (req.url === '/chunks') { + res.writeHead(200) + res.write(testContentBigGzip.slice(0, 4096)) + setTimeout(function() { res.end(testContentBigGzip.slice(4096)) }, 10) } else { zlib.gzip(testContent, function(err, data) { assert.equal(err, null) @@ -30,11 +36,30 @@ var server = http.createServer(function(req, res) { }) tape('setup', function(t) { + // Need big compressed content to be large enough to chunk into gzip blocks. + // Want it to be deterministic to ensure test is reliable. + // Generate pseudo-random printable ASCII characters using MINSTD + var a = 48271 + , m = 0x7FFFFFFF + , x = 1 + testContentBig = new Buffer(10240) + for (var i = 0; i < testContentBig.length; ++i) { + x = (a * x) & m + // Printable ASCII range from 32-126, inclusive + testContentBig[i] = (x % 95) + 32 + } + zlib.gzip(testContent, function(err, data) { t.equal(err, null) testContentGzip = data - server.listen(6767, function() { - t.end() + + zlib.gzip(testContentBig, function(err, data2) { + t.equal(err, null) + testContentBigGzip = data2 + + server.listen(6767, function() { + t.end() + }) }) }) }) @@ -139,6 +164,50 @@ tape('transparently supports gzip error to pipes', function(t) { }) }) +tape('pause when streaming from a gzip request object', function(t) { + var chunks = [] + var paused = false + var options = { url: 'http://localhost:6767/chunks', gzip: true } + request.get(options) + .on('data', function(chunk) { + var self = this + + t.notOk(paused, 'Only receive data when not paused') + + chunks.push(chunk) + if (chunks.length === 1) { + self.pause() + paused = true + setTimeout(function() { + paused = false + self.resume() + }, 100) + } + }) + .on('end', function() { + t.ok(chunks.length > 1, 'Received multiple chunks') + t.ok(Buffer.concat(chunks).equals(testContentBig), 'Expected content') + t.end() + }) +}) + +tape('pause before streaming from a gzip request object', function(t) { + var paused = true + var options = { url: 'http://localhost:6767/foo', gzip: true } + var r = request.get(options) + r.pause() + r.on('data', function(data) { + t.notOk(paused, 'Only receive data when not paused') + t.equal(data.toString(), testContent) + }) + r.on('end', t.end.bind(t)) + + setTimeout(function() { + paused = false + r.resume() + }, 100) +}) + tape('cleanup', function(t) { server.close(function() { t.end() diff --git a/tests/test-pipes.js b/tests/test-pipes.js index 0e4be002c..3763f5781 100644 --- a/tests/test-pipes.js +++ b/tests/test-pipes.js @@ -131,6 +131,66 @@ tape('piping from a request object', function(t) { } }) +tape('pause when piping from a request object', function(t) { + s.once('/chunks', function(req, res) { + res.writeHead(200, { + 'content-type': 'text/plain' + }) + res.write('Chunk 1') + setTimeout(function() { res.end('Chunk 2') }, 10) + }) + + var chunkNum = 0 + var paused = false + request({ + url: s.url + '/chunks' + }) + .on('data', function(chunk) { + var self = this + + t.notOk(paused, 'Only receive data when not paused') + + ++chunkNum + if (chunkNum === 1) { + t.equal(chunk.toString(), 'Chunk 1') + self.pause() + paused = true + setTimeout(function() { + paused = false + self.resume() + }, 100) + } else { + t.equal(chunk.toString(), 'Chunk 2') + } + }) + .on('end', t.end.bind(t)) +}) + +tape('pause before piping from a request object', function(t) { + s.once('/pause-before', function(req, res) { + res.writeHead(200, { + 'content-type': 'text/plain' + }) + res.end('Data') + }) + + var paused = true + var r = request({ + url: s.url + '/pause-before' + }) + r.pause() + r.on('data', function(data) { + t.notOk(paused, 'Only receive data when not paused') + t.equal(data.toString(), 'Data') + }) + r.on('end', t.end.bind(t)) + + setTimeout(function() { + paused = false + r.resume() + }, 100) +}) + var fileContents = fs.readFileSync(__filename).toString() function testPipeFromFile(testName, hasContentLength) { tape(testName, function(t) {