Fix setting content-length: 0 for HEAD responses with compression middleware

[davidpdrsn]

Fixes #747

The bug was that applying a middleware to a method router would cause the response to pass through a RouteFuture twice and we'd set the content-length and strip the response body twice. That happened because Router::layer wrapped things in a Route then called MethodRoute::route which also applied a Route.

Solved it by having RouteFuture::poll set a response extension and not alter the response if that is present. Its a little unfortunate to add more overhead here but couldn't think of another solution since Router and MethodRouter need to be correct and make sense on their own, and needs to support general services that might not handle HEAD requests correctly.

I also removed RouterFuture so Router's response is now RouteFuture<RequestBody, Infallible>. The additional indirection of RouterFuture hasn't proven to be useful so I think its safe to remove.

[jplatte]

I don't think I really understand what went wrong here. content-length setting should only happen if there is no content-length yet, right?

[davidpdrsn]

I understand that its unclear, honestly it wasn't clear to me either. I've done some more digging and think I understand whats going on now:

If you have a router like this:

Router::new()
    .route("/", get(|| async { "Hello, World!" }))
    .layer(CompressionLayer::new().compress_when(SizeAbove::new(0)))

run it with hyper and send it HEAD / with accept-encoding: gzip the flow will be:

• Router receives the request, does its routing, and passes it to the matching route
• That route has a Compression middleware applied but it doesn't change the request, just checks for accept-encoding
• The MethodRouter is called which yields a RouteFuture
• This route future is wrapped in Compression's response future
• That response future is wrapped in RouteFuture again when it is returned from Router
• So the future stack is RouteFuture<CompressionFuture<RouteFuture<Handler>>>. Of course these types kind actually exist like that but hopefully you understand.
• The handler future yields a response as normal
• The first, inner most, RouteFuture sees that the request was a HEAD so it sets the content-length (since the body returned by the handler had an exact known size) and then sets the body to Empty
• The compression future removes the content-length (since you cannot have transfer-encoding: chunked + content-length)
• Now the response arrives at the outer most RouteFuture which first tries to sent the content-length but can't since the body doesn't have a know size. That is CompressionBody doesn't impl size_hint. However this future will set the body to Empty again.
• hyper will see that there is no content-length and check if the body has a exact size, which Empty does and so hyper sets the content-length to 0.

I think the most correct thing is to omit the content-length since if the request is issued again with GET then it wont have a content-length, because it'll have transfer-encoding: chunked, since its compressed on the fly.

Phew that was a lot 😅 Hopefully it makes sense.

[jplatte]

I see two bugs there:

• The compression layer should not do anything in this case since the body is actually empty. Maybe ignore an already-set content-length header when the method is HEAD / OPTIONS, or don't do anything on those methods even without the compress_when option.
• Hyper shouldn't be setting content-length when encountering transfer-encoding: chunked, if that combination is illegal.

[davidpdrsn]

The compression layer should not do anything in this case since the body is actually empty. Maybe ignore an already-set content-length header when the method is HEAD / OPTIONS, or don't do anything on those methods even without the compress_when option.

The compression middleware was configured with .compress_when(SizeAbove::new(0)) which is a bit silly in the first place. By default it wont compress empty bodies. But I agree its probably better that compression doesn't do anything on HEAD requests no matter what. I think thats a separate bug though.

Hyper shouldn't be setting content-length when encountering transfer-encoding: chunked, if that combination is illegal.

Hyper does the right thing afaik. Hyper calls http_body::Body::size_hint and sees an extract of 0 and sets content-length: 0. There is no transfer-encoding in the response. Compression actually doesn't set transfer-encoding: chunked, it leaves that up to hyper as hyper does the actual encoding of the body into the stream.

[davidpdrsn]

If you send a HEAD request the response is

HTTP/1.1 200 OK
content-type: text/plain; charset=utf-8
content-encoding: gzip
date: Sun, 13 Feb 2022 08:40:12 GMT

with no body. Before this patch this response would have content-length: 0 which is wrong since changing the method to GET wont return an empty response.

With this patch the response to GET is

HTTP/1.1 200 OK
content-type: text/plain; charset=utf-8
content-encoding: gzip
transfer-encoding: chunked
date: Sun, 13 Feb 2022 08:41:14 GMT

21
��H�����/�IQ��J�
0

This is unchanged by this patch

So transfer-encoding: chunked is missing from the HEAD response but that feels correct as per this from MDN:

Transfer-Encoding is a hop-by-hop header, that is applied to a message between two nodes, not to a resource itself.

[jplatte]

The compression middleware was configured with .compress_when(SizeAbove::new(0)) which is a bit silly in the first place.

But 0 isn't above 0 so it should still not compress.. Is SizeAbove really SizeGreaterOrEqual?

[jplatte]

I think thats a separate bug though.

Can you think of other reasonable middlewares / overall setups that would cause the same issue?

[davidpdrsn]

But 0 isn't above 0 so it should still not compress.. Is SizeAbove really SizeGreaterOrEqual?

That's true. I think that's a bug in the implementation. The docs says "above a certain size" so I don't think it's a breaking change to fix.

Can you think of other reasonable middlewares / overall setups that would cause the same issue?

Decompression probably does. tower-http doesn't have other middleware that change the body.

[jplatte]

I'm not really convinced axum is currently doing anything wrong, but clearly this is a way of solving #747 so if you want to go with it, I don't wanna block that.