compression
compression copied to clipboard
expressjs
callback parameter is missing from 'write' and 'end' methods
The methods 'write' and 'end' don't accept a 'callback' parameter - the underlaying http response stream does. Therefore there is a different runtime behaviour when sending off the data in chunks (waiting for the response to be written) when using compression (callback will not be called and the program hangs)
Hi! What version of Node.js are you using? It doesn't seem to exist in 0.10, at least.
Hi!
I'm using 0.12 (https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback)
But I guess this parameter was there all along. That's the node way :-) (https://nodejs.org/docs/v0.10.0/api/stream.html#stream_writable_write_chunk_encoding_callback_1)
This module is only for HTTP streams, which in Node.js are not actually the normal streams at all. For example, this is the actual source for the write method we are overwriting in 0.10: https://github.com/joyent/node/blob/v0.10.24/lib/http.js#L841 and as you can see, it takes no callback argument.
It looks like the callback argument was added in Node.js 0.12 sometime: https://github.com/joyent/node/blob/v0.12.0/lib/_http_outgoing.js#L409
Looks like the callback argument to .write was added in Node.js 0.11.6: https://github.com/joyent/node/commit/da93d6adfb0abfcaac26e1509748edca0db8c003
I'm using Node 0.10.40. I supply the callback on second argument in res.write() and It's work but when compression middleware it throws an error:
TypeError: Unknown encoding: function (err){
if(err){
return console.log(err);
}
res.flush();
stream.resume();
}
which is my callback function. I try to fix it by change second arguments to "utf-8" and move callback to third argument. The error was gone but callback never get call.
update: I use node 4.1.2. Callback not get call too.
I just ran into this problem as well and it took me a long time to figure out what was going on as it unexpectedly breaks res.write in all middleware after this middleware is loaded. This bug prevents this middleware from being node 11+ compliant. If you ever add the compression middleware, all future calls to res.write no longer call the callback.
var express = require('express');
var compression = require('compression')
var app = express();
app.use(compression())
app.get('/', function (req, res) {
res.write('Hello World!', 'utf-8', function() {
console.log('will never be called since compression overrode res.write without the callback parameter');
});
res.end();
});
app.listen(3000);
This is worse if you put the res.end inside of the res.write callback as your server will hang indefinitely.
Is there any update as to when this will be fixed? It seems simple enough of a fix if you just used _write.apply(res, arguments) in your res.write override so that all arguments will be preserved to the previous write function.
There is the PR #80 that is actively being worked on that would resolve this.
There seems to be a lot going on in that PR discussion wise. From what I can gather you seem to want two different PRs made: one that fixes this bug specifically and the other that fixes the flush() bug. If I create a PR to fix this bug, do you see any problem with changing the res.write and res.end calls to use apply to pass through the arguments properly?
Hi @straker, yes there is a lot of discussion. The reference PR is specific to add the callbacks; the second PR to fix the flush bug was never made.
If I create a PR to fix this bug, do you see any problem with changing the res.write and res.end calls to use apply to pass through the arguments properly?
So there is more to fixing it than that, and there has been a lot of discussion around that very point in the PR :) The main point is that you cannot just pass the callbacks through, because otherwise, it will make the callbacks even more unpredictable on Node.js 0.8 and 0.10, which changes this module from "if you use this, you cannot use callbacks" to "if you use this, you can use callbacks, except on Node.js 0.8 and 0.10, where normally you cannot use callbacks, but now you can, but not if the client sends headers to not compress the output".
Ah, I thought that PR was to fix the flush bug.
I'm not sure if I understand what you mean. How does passing through the arguments from compressions res.write to nodes res.write now allow callbacks in Node 0.8 and 0.10 if the res.write node function doesn't accept a 3rd parameter in those version? All that would mean is that it receives an additional parameter that it ignores.
I thought that's what the two different PRs were suppose to separate because the PR you referenced looks(ed) at the argument list to compression's res.write and then manually calls the callback if it exists. Changing compressions override of res.write to call _write.apply(res, arguments) instead of _write.call(this, chunk, encoding) at the end still does not allow Node 0.8 and 0.10 to use callbacks (since the _write does not accept them), but does allow Node 0.11+ to use them as per the spec. No functionality would be changed in those 2 versions of Node.
Ah, I thought that PR was to fix the flush bug.
No, it doesn't, which was the point I was bringing up, that the PR is just trying to add callback support instead of actually fixing the flush bug.
How does passing through the arguments from compressions res.write to nodes res.write now allow callbacks in Node 0.8 and 0.10 if the res.write node function doesn't accept a 3rd parameter in those version?
Because the zlib stream in 0.8 and 0.10 does support the callbacks. You're welcome to also make a pull request if you have a different approach, but every time you keep talking about calling _write, that would only apply to non-compressed responses, as compressed responses write to the zlib stream.
I see, I misunderstood how that all worked together. I can see how that complicates maters significantly. I'll investigate further to see if I can come up with something else that will work taking into account zlib. Thanks for the responses.
No problem, @straker! I would love to get this fixed, where it doesn't introduce even more head-aches for debugging :S I just have had so many more higher-priority issues to work around, and besides from this initial issue, I never even heard from anyone besides two just suddenly that it was even a big issue (probably because, when surveying npm yesterday, almost every single module that overwrites res.write/res.end does not even pass through the callbacks, either, so it seems to be a fairly low-used API (at least among the users of those modules).
The Express.js TC has spoken about Node.js support recently, and we are wiling to start bumping majors to drop Node.js 0.8 support, but with Node.js 0.10 still supported by the Node.js foundation (which we are a part of) and still demand for Node.js 0.10 support among corporate, dropping 0.10 is still a bit iffy. I bring that up because dropping those versions would make this a non-issue...