Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix bug in compress detecting content-type set via writeHead #524

Closed
wants to merge 1 commit into from

5 participants

@nateps

The compress middleware (and presumably other middleware that respond to the 'header' event) did not properly detect headers set via writeHead. Updates the writeHead patch to use setHeader to set any headers before emitting this event.

For reference to Node's handling of setHeader arguments, see: https://github.com/joyent/node/blob/master/lib/http.js#L911

@tj
Owner
tj commented

blah, writeHead sucks, node is supposed to be normalizing all of this internally at some point but i'll take a look

@tj tj commented on the diff
test/compress.js
@@ -7,6 +7,38 @@ var app = connect();
app.use(connect.compress());
app.use(connect.static(fixtures));
+var dynamicApp = connect();
+dynamicApp.use(connect.compress());
+
+dynamicApp.use(function(req, res){
+ var body = '- groceries';
@tj Owner
tj added a note

I think the tests should be in test/patch.js since that's what we're changing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@defunctzombie

I am running into this issue as well. The interesting note in the docs for res.getHeader

Reads out a header that's already been queued but not sent to the client. Note that the name is case insensitive. This can only be called before headers get implicitly flushed.

Which means that using it like such in compress is probly not a good idea since it may not be available if already sent to the client. This is certainly quite annoying and am not sure why node does this instead of just tracking what it has already sent.

@fmarier

I've run into the same problem. Here was my initial rough attempt at fixing it: fmarier@49bd656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 80 additions and 2 deletions.
  1. +18 −2 lib/patch.js
  2. +62 −0 test/compress.js
View
20 lib/patch.js
@@ -69,10 +69,26 @@ if (!res._hasConnectPatch) {
return _renderHeaders.call(this);
};
- res.writeHead = function(){
+ res.writeHead = function(statusCode){
+ var reasonPhrase, headers, headerIndex;
+
+ if (typeof arguments[1] == 'string') {
+ reasonPhrase = arguments[1];
+ headerIndex = 2;
+ } else {
+ headerIndex = 1;
+ }
+ headers = arguments[headerIndex];
+
+ if (headers) {
+ for (var name in headers) {
+ this.setHeader(name, headers[name]);
+ }
+ }
+
if (!this._emittedHeader) this.emit('header');
this._emittedHeader = true;
- return writeHead.apply(this, arguments);
+ return writeHead.call(this, statusCode, reasonPhrase);
};
res._hasConnectPatch = true;
View
62 test/compress.js
@@ -7,6 +7,38 @@ var app = connect();
app.use(connect.compress());
app.use(connect.static(fixtures));
+var dynamicApp = connect();
+dynamicApp.use(connect.compress());
+
+dynamicApp.use(function(req, res){
+ var body = '- groceries';
@tj Owner
tj added a note

I think the tests should be in test/patch.js since that's what we're changing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ if (req.url == '/setHeaderEnd') {
+ res.setHeader('Content-Type', 'text/plain')
+ res.end(body);
+ return
+ }
+
+ if (req.url == '/writeHeadEnd') {
+ res.writeHead(200, {
+ 'Content-Length': body.length,
+ 'Content-Type': 'text/plain'
+ });
+ res.end(body);
+ return
+ }
+
+ if (req.url == '/writeHeadWriteEnd') {
+ res.writeHead(200, {
+ 'Content-Length': body.length,
+ 'Content-Type': 'text/plain'
+ });
+ res.write(body)
+ res.end();
+ return
+ }
+});
+
describe('connect.compress()', function(){
it('should gzip files', function(done){
app.request()
@@ -18,6 +50,36 @@ describe('connect.compress()', function(){
});
})
+ it('should gzip after setHeader(), end()', function(done){
+ dynamicApp.request()
+ .get('/setHeaderEnd')
+ .set('Accept-Encoding', 'gzip')
+ .end(function(res){
+ res.body.should.not.equal('- groceries');
+ done();
+ });
+ })
+
+ it('should gzip after writeHead(), end()', function(done){
+ dynamicApp.request()
+ .get('/writeHeadEnd')
+ .set('Accept-Encoding', 'gzip')
+ .end(function(res){
+ res.body.should.not.equal('- groceries');
+ done();
+ });
+ })
+
+ it('should gzip after writeHead(), write(), end()', function(done){
+ dynamicApp.request()
+ .get('/writeHeadWriteEnd')
+ .set('Accept-Encoding', 'gzip')
+ .end(function(res){
+ res.body.should.not.equal('- groceries');
+ done();
+ });
+ })
+
it('should set Content-Encoding', function(done){
app.request()
.get('/todo.txt')
Something went wrong with that request. Please try again.