Merge pull request #1569 from kevinoid/fix-gzip-pause

Fix pause before response arrives

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()
   }
 

package.json

@@ -49,6 +49,7 @@
   "devDependencies": {
     "browserify": "~5.9.1",
     "browserify-istanbul": "~0.1.3",
+    "buffer-equal": "0.0.1",
     "coveralls": "~2.11.2",
     "eslint": "0.18.0",
     "function-bind": "~1.0.0",

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)

tests/test-gzip.js

@@ -4,9 +4,12 @@ var request = require('../index')
   , http = require('http')
   , zlib = require('zlib')
   , assert = require('assert')
+  , bufferEqual = require('buffer-equal')
   , tape = require('tape')
 
 var testContent = 'Compressible response content.\n'
+  , testContentBig
+  , testContentBigGzip
   , testContentGzip
 
 var server = http.createServer(function(req, res) {
@@ -18,6 +21,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 +37,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 +165,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(bufferEqual(Buffer.concat(chunks), 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()

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) {