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

[expressjs]

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

[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.