Fix pause before response

Due to request#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 <kevin@kevinlocke.name>

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

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

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

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