warp: Connections with HEAD requests are not kept alive if they don't have Content-Length

[akshaymankar]

Here is a tiny server for testing:

{-# LANGUAGE OverloadedStrings #-}

import Network.Wai.Handler.Warp qualified as Warp
import Network.Wai
import Network.HTTP.Types

main :: IO ()
main = Warp.run 3000 testApp

testApp :: Application
testApp req respond =
  respond $ responseLBS status200 [] ""

Any requests made to this server keep the connection alive except for HEAD requests.

For client side I used telnet. Send a `GET` request like this (note the extra new lines):

GET / HTTP/1.1

The connection will hang on. Then send a HEAD request like this:

HEAD / HTTP/1.1

The connection will be closed by the server.

I've tracked it down to these two lines:

wai/warp/Network/Wai/Handler/Warp/Response.hs

Line 143 in 2eb5339

isChunked = not isHead && isChunked0

wai/warp/Network/Wai/Handler/Warp/Response.hs

Line 386 in 2eb5339

isKeepAlive = isPersist && (isChunked || hasLength)

So, isChunked is always False for HEAD requests and it depends completely on the Content-Length header in response.
This is not what nginx expects and this causes a race between warp actually closing the connection and nginx using it again causing nginx sometimes return 502 Bad Gateway. It also logs errors like this:

[error] 1198525#1198525: *13 upstream prematurely closed connection while reading response header from upstream,

I am not sure why the connection is closed on this. But I also noticed that when warp decides to close a connection it doesn't include the Connection: close header to tell the client that it shouldn't try to re-use the connection.

[Vlix]

Reading through RFC9110, it does say "However, a server MAY omit header fields for which a value is determined only while generating the content. [...] Such a response to GET might contain Content-Length and Vary fields, for example, that are not generated within a HEAD response."

So I think removing the not isHead would be an acceptable change, right?

[kazu-yamamoto]

I don't remember why I gave special treatment to the HEAD method.
It was probably because of my misunderstanding.
I agree with @Vlix's observation.

[Vlix]

Note for posterity, isChunked is basically just an isHttp11 check.

[Vlix]

I've looked through RFC-2616 to check if HEAD was maybe handled differently in the past, but couldn't find any mention of it. So it is probably just a misunderstanding.

A PR would be welcomed to rectify this.

EDIT: Since it's in the isChunked definition, I looked around and other people were asking about HEAD + chunked and maybe this happened because the Transfer-Encoding header should be omitted for HEAD responses (though they MAY be included) because no body means no encoding. (and thus "chunked" is "false")