Skip to content

Commit

Permalink
Pause/Resume response content, not response
Browse files Browse the repository at this point in the history
When the response content is piped through additional streams for
decoding, pause and resume calls should be propagated to the last stream
in the pipeline so that back pressure propagates correctly.

This avoids an issue where simultaneous back pressure from the content
decoding stream and from a stream to which Request is piped could cause
the response stream to get stuck waiting for a drain event on the
content decoding stream which never occurs.  See request#1567 for an example.

This commit also renames dataStream to responseContent to remedy my
previous poor choice of name, since the name will be exposed on the
Request instance it should be clearer and closer to the name used to
refer to this data in the relevant RFCs.

Fixes request#1567

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
  • Loading branch information
kevinoid committed May 8, 2015
1 parent 8382742 commit b117c59
Showing 1 changed file with 18 additions and 16 deletions.
34 changes: 18 additions & 16 deletions request.js
Expand Up @@ -1120,56 +1120,58 @@ Request.prototype.onRequestResponse = function (response) {
self._ended = true
})

var dataStream
var responseContent
if (self.gzip) {
var contentEncoding = response.headers['content-encoding'] || 'identity'
contentEncoding = contentEncoding.trim().toLowerCase()

if (contentEncoding === 'gzip') {
dataStream = zlib.createGunzip()
response.pipe(dataStream)
responseContent = zlib.createGunzip()
response.pipe(responseContent)
} else {
// Since previous versions didn't check for Content-Encoding header,
// ignore any invalid values to preserve backwards-compatibility
if (contentEncoding !== 'identity') {
debug('ignoring unrecognized Content-Encoding ' + contentEncoding)
}
dataStream = response
responseContent = response
}
} else {
dataStream = response
responseContent = response
}

if (self.encoding) {
if (self.dests.length !== 0) {
console.error('Ignoring encoding parameter as this stream is being piped to another stream which makes the encoding option invalid.')
} else if (dataStream.setEncoding) {
dataStream.setEncoding(self.encoding)
} else if (responseContent.setEncoding) {
responseContent.setEncoding(self.encoding)
} else {
// Should only occur on node pre-v0.9.4 (joyent/node@9b5abe5) with
// zlib streams.
// If/When support for 0.9.4 is dropped, this should be unnecessary.
dataStream = dataStream.pipe(stringstream(self.encoding))
responseContent = responseContent.pipe(stringstream(self.encoding))
}
}

self.responseContent = responseContent

self.emit('response', response)

self.dests.forEach(function (dest) {
self.pipeDest(dest)
})

dataStream.on('data', function (chunk) {
responseContent.on('data', function (chunk) {
self._destdata = true
self.emit('data', chunk)
})
dataStream.on('end', function (chunk) {
responseContent.on('end', function (chunk) {
self.emit('end', chunk)
})
dataStream.on('error', function (error) {
responseContent.on('error', function (error) {
self.emit('error', error)
})
dataStream.on('close', function () {self.emit('close')})
responseContent.on('close', function () {self.emit('close')})

if (self.callback) {
var buffer = bl()
Expand Down Expand Up @@ -1536,18 +1538,18 @@ Request.prototype.end = function (chunk) {
}
Request.prototype.pause = function () {
var self = this
if (!self.response) {
if (!self.responseContent) {
self._paused = true
} else {
self.response.pause.apply(self.response, arguments)
self.responseContent.pause.apply(self.responseContent, arguments)
}
}
Request.prototype.resume = function () {
var self = this
if (!self.response) {
if (!self.responseContent) {
self._paused = false
} else {
self.response.resume.apply(self.response, arguments)
self.responseContent.resume.apply(self.responseContent, arguments)
}
}
Request.prototype.destroy = function () {
Expand Down

0 comments on commit b117c59

Please sign in to comment.