Compression returns (invalid) Content-Length which hangs HTTP connections.

[wrr]

The middleware does not process and modify headers that are passed as arguments to the writeHead() function. If the Content-Length is passed to the writeHead(), it is returned unaltered, with a value that after compression is invalid.

A breaking example:

var connect = require('connect');
var compress = require('compression');
var http = require('http');

var app = connect()
  .use(compress({
    threshold: 0
  }))
  .use(function(req, res) {
    res.setHeader('Content-Type', 'text/plain')
    res.writeHead(200, {'Content-Length': 3})
    res.end('foo');
  });

http.createServer(app).listen(5000);

This returns following headers:

Connection:keep-alive
Content-Encoding:gzip
Content-Length:3
Content-Type:text/plain
Date:Wed, 26 Mar 2014 19:16:26 GMT
Vary:Accept-Encoding

A failing unit test

There are other related issues caused by the same bug. Following headers are incorrectly processed if they are passed to the writeHead function: Vary, Content-Type, Content-Encoding.

[jonathanong]

PR welcome, but IMO .writeHead() is a terrible api that people should avoid.

[dougwilson]

Also, @wrr if you are going to make a PR, please create a new test that fails, rather than modifying an existing test.

[Fishrock123]

@wrr Any middleware cannot process things that further middleware does.

Your breaking example will always fail. (not the repurposed the unit test)

Furthermore, I have seen the use of writeHead discouraged in mainstream use by node's maintainers, and you should probably avoid it.

[wrr]

writeHead is a part of a stable node API, it is not deprecated and middleware should not break it. writeHead isn't also discouraged in the official node documentation. If compression does not to support writeHead, it IMO should explicitly fail (with an assertion in the writeHead method). But compression tries to implement writeHead, the implementation is broken and your argument is that it is fine because writeHead API is ugly?

[Fishrock123]

Oh. I'm wrong. We actually overwrite res.writeHead so that using it after compression works properly. That means that your unit test should fail (because we don't handle that). Your example should also fail.

We only call node's writeHead after everything else, is an issue?
https://github.com/expressjs/compression/blob/master/index.js#L141-L156

As far as I can tell, we aren't breaking it.

[wrr]

In my example, if you remove compression middleware, the example works fine, if you add the middleware, it returns invalid response. This means that the middleware does not support writeHead with non empty headers argument and breaks code that uses this API. Returning invalid responses is not the same as failing. If compressions failed (with an exception), it would be easy to detect that returning headers in writeHead is not supported.

[Fishrock123]

Of course res.writeHead(200, {'Content-Length': 3}) is going to cause an invalid response, what do you expect?

[wrr]

My expectations are the same as with res.setHeader('Content-Length', 3) which does not cause an invalid response. Compression middleware is altering length of content, and it should handle changing Content-Length headers (or replacing them with chunked encoding) in such a way that returned response remains valid. It does so correctly when headers are passed with setHeader(), it doesn't do it when headers are passed with writeHead().

Are you arguing that there is no bug? My impression was that @jonathanong acknowledged the bug, but pointed that he isn't in favor of supporting 'writeHead` API.

[Fishrock123]

I'm going with: I do not understand this enough and someone else will probably have to fix it.

Apologies.

[dougwilson]

This thread is soo annoying. It's fixed now, you're welcome.