Skip to content

Adding callback to res.write and res.end for streaming support #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 20 commits into from
Closed

Adding callback to res.write and res.end for streaming support #80

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ed51c3a
Update index.js
May 8, 2016
f9c95f9
Update index.js
May 8, 2016
c4ba4af
Adding tests.
jpodwys May 23, 2016
bd60b96
Updating docs.
jpodwys May 23, 2016
5cd1fe2
no message
jpodwys May 23, 2016
2b85837
no message
jpodwys May 23, 2016
31792c1
Testing with and without compression.
jpodwys May 23, 2016
bda469c
Reverting readme change and ensuring callbacks are called in the orde…
jpodwys May 23, 2016
5f122e8
Adding error handling.
jpodwys May 23, 2016
eb1bad8
Fixing lint errors.
jpodwys May 23, 2016
ac8914b
Adding test with no streaming.
jpodwys May 24, 2016
866775d
no message
jpodwys May 24, 2016
c1f3aab
Attempting to handle pre-11 node versions.
jpodwys May 24, 2016
b0055d2
no message
jpodwys May 24, 2016
3ab2f0d
Handling node 0.8 and 0.10.
jpodwys May 24, 2016
79c6b75
Typo.
jpodwys May 24, 2016
0f617c7
Checking OutgoingMessage.prototype.
jpodwys May 24, 2016
4b50f90
Making res.flush synchronous.
jpodwys May 28, 2016
2e995d0
no message
jpodwys May 28, 2016
74fdf7b
no message
jpodwys May 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ function compression (options) {
var length
var listeners = []
var stream
var noop = function () {}

var _end = res.end
res._end = res.end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot store the references as res._end and _write. There is no code where that check nothing is being overwritten, for example. Using this module twice will no longer function, etc.

Doing this causes too many bugs to be able to accept. Why did you have to change this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that doing this isn't going to work, but I documented why I did it in the comment just above this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll notice that in my latest commit, I attached _write and _end to res . I did this so that I can determine from the unit tests whether the pre-existing .write() and .end() calls accept a callback parameter.

Ah, sorry, I missed that. Unfortunately that is not an acceptable solution, due to the bugs it is causing from actual real-world apps I dropped your PR into in the last few hours. There is other code from npm that has written to those variables and responses are hanging or causing a stack overflow now.

You will need you use a different method for that check in the tests, not compromise the self-contained nature of this module. For example, add a middleware prior to this on in the tests and check against that.

In fact, the necessity to do this seems like a critical flaw to this functionality, for example, another module overwrites the moths with the three argument form, and just passes them upstream. This will now erroneously think callbacks are supported and we end up in the same buggy situation we set out to solve. It seems the method of using function length is just too fragile and I encourage you to come up with a different solution.

res._write = res.write
var _on = res.on
var _write = res.write

// flush
res.flush = function flush () {
Expand All @@ -74,25 +75,29 @@ function compression (options) {

// proxy

res.write = function write (chunk, encoding) {
res.write = function write (chunk, encoding, cb) {
if (ended) {
return false
}

cb = (res._write.length === 3) ? cb : null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is going yo be robust, for example, I see the following pretty frequently:

res.write = function write () {
  // done
  return _write.apply(this, arguments)
}

This is a very common pattern to wrap some function, but the length of the function ends up being zero, which means this module won't send the callback (and the wrapping may only be for certain requests, complicating debugging, which would point to "when I use compression, my callbacks sometimes do not work. Removing compression works fine").

Thoughts?


if (!this._header) {
this._implicitHeader()
}

return stream
? stream.write(new Buffer(chunk, encoding))
: _write.call(this, chunk, encoding)
? stream.write(new Buffer(chunk, encoding), cb)
: res._write.call(this, chunk, encoding, cb)
}

res.end = function end (chunk, encoding) {
res.end = function end (chunk, encoding, cb) {
if (ended) {
return false
}

cb = (res._end.length === 3) ? cb : null

if (!this._header) {
// estimate the length
if (!this.getHeader('Content-Length')) {
Expand All @@ -103,16 +108,16 @@ function compression (options) {
}

if (!stream) {
return _end.call(this, chunk, encoding)
return res._end.call(this, chunk, encoding, cb)
}

// mark ended
ended = true

// write Buffer for Node.js 0.8
return chunk
? stream.end(new Buffer(chunk, encoding))
: stream.end()
? stream.end(new Buffer(chunk, encoding), null, cb)
: stream.end(null, null, cb)
}

res.on = function on (type, listener) {
Expand Down Expand Up @@ -202,13 +207,13 @@ function compression (options) {

// compression
stream.on('data', function onStreamData (chunk) {
if (_write.call(res, chunk) === false) {
if (res._write.call(res, chunk) === false) {
stream.pause()
}
})

stream.on('end', function onStreamEnd () {
_end.call(res)
res._end.call(res)
})

_on.call(res, 'drain', function onResponseDrain () {
Expand Down
70 changes: 70 additions & 0 deletions test/compression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,76 @@ describe('compression()', function () {
.end()
})
})

describe('when callbacks are used', function () {
it('should call the passed callbacks in the order passed when compressing', function (done) {
var hasCallbacks = false
var callbackOutput = []
var server = createServer(null, function (req, res) {
hasCallbacks = (res._write.length === 3 && res._end.length === 3)
res.setHeader('Content-Type', 'text/plain')
res.write('Hello', null, function () {
callbackOutput.push(0)
})
res.write(' World', null, function () {
callbackOutput.push(1)
})
res.end(null, null, function () {
callbackOutput.push(2)
})
})

request(server)
.get('/')
.set('Accept-Encoding', 'gzip')
.expect('Content-Encoding', 'gzip')
.end(function (err) {
if (err) {
throw new Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to wrap an error ina new error, because it looses the original stack trace. It is good practice to test your error handling code to see that it works, just like the normal code. This should simply be passing err to the done function.

}
if (hasCallbacks) {
assert.equal(callbackOutput.length, 3)
assert.deepEqual(callbackOutput, [0, 1, 2])
}
done()
})
})

it('should call the passed callbacks in the order passed when not compressing', function (done) {
var hasCallbacks = false
var callbackOutput = []
var server = createServer(null, function (req, res) {
hasCallbacks = (res._write.length === 3 && res._end.length === 3)
res.setHeader('Cache-Control', 'no-transform')
res.setHeader('Content-Type', 'text/plain')
res.write('hello,', null, function () {
callbackOutput.push(0)
})
res.write(' world', null, function () {
callbackOutput.push(1)
})
res.end(null, null, function () {
callbackOutput.push(2)
})
})

request(server)
.get('/')
.set('Accept-Encoding', 'gzip')
.expect('Cache-Control', 'no-transform')
.expect(shouldNotHaveHeader('Content-Encoding'))
.end(function (err) {
if (err) {
throw new Error(err)
}
if (hasCallbacks) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test when hasCallbacks is false is flawed. Instead of verifying that no callbacks were called, it does not checks at all, basically it's as if the test didn't even run. You need to add tests for that condition as well. Probably assert that callbackOutput is empty, as expected.

assert.equal(callbackOutput.length, 3)
assert.deepEqual(callbackOutput, [0, 1, 2])
}
done()
})
})
})
})

function createServer (opts, fn) {
Expand Down