No Content responses can have content-length but no body

[mikeshepherd]

A custom No Content response, that incorrectly includes a body and an explicit Content-Length header, results in an inconsistent http response.
Ember server seems to drop the body, but leaves the Content-Length header. This can cause some clients to timeout waiting for content that never arrives. Since ember already ignores the body it seems like it wouldn't be a stretch to also ignore the given Content-Length header as well (conceptually anyway 😀 , I've no idea about the code change required to make this happen, if someone can point me in the right direction I'm happy to work on it and raise a PR).

Depending on how a No Content response is constructed, and how it is exercised, the response can vary.

I've created a reproduction of this here: https://gist.github.com/mikeshepherd/10ab7e5c2cfb933c587f3e01bf1c06a6

Edit: This was discovered while using smithy4s, which generates http4s code, making it hard to just use the standard NoContent response. See disneystreaming/smithy4s#1169

[armanbilge]

Thanks!

if someone can point me in the right direction I'm happy to work on it and raise a PR

Maybe around here? Not sure what's the best way to handle this.

http4s/ember-core/shared/src/main/scala/org/http4s/ember/core/Encoder.scala

Lines 65 to 70 in ce7de6b

	def isEmptyBody = resp.body eq EmptyBody
	def isEntityAllowed = resp.status.isEntityAllowed
	if (!appliedContentLength && isEmptyBody && isEntityAllowed) {
	stringBuilder.append(zeroContentLengthRaw).append(CRLF)
	chunked = false
	} else if (!chunked && !appliedContentLength && isEntityAllowed) {

[mikeshepherd]

Not sure what's the best way to handle this

Indeed 😀

I did find that chunk of code but wasn't sure I'd followed the correct path as it seems to always include the body, I can't figure out where that gets dropped.

From the bottom of that function:

http4s/ember-core/shared/src/main/scala/org/http4s/ember/core/Encoder.scala

Lines 79 to 86 in ce7de6b

	if (chunked)
	Stream.chunk(Chunk.array(initSection)) ++ resp.body.through(
	ChunkedEncoding.encode[F](resp.trailerHeaders)
	)
	else
	(Stream.chunk(Chunk.array(initSection)) ++ resp.body)
	.chunkMin(writeBufferSize)
	.unchunks

Naively it seems like wrapping
if (!(h.name == `Content-Length`.name && resp.body eq EmptyBody)) around the contents of this loop would supress the header.

http4s/ember-core/shared/src/main/scala/org/http4s/ember/core/Encoder.scala

Lines 55 to 62 in ce7de6b

	resp.headers.foreach { h =>
	if (h.isNameValid) {
	appliedContentLength = appliedContentLength || h.name == `Content-Length`.name
	stringBuilder.append(h.name).append(": ")
	appendSanitized(stringBuilder, h.value)
	stringBuilder.append(CRLF)
	}

[mikeshepherd]

I'm inclined close this issue as I don't think it behaves like I originally thought.
The server returns the data in both cases, it's just masked by the client.

This can be shown by using netcat to craft a http request: e.g.
printf 'GET /example HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n' | nc localhost 8080
with changes to my example to leave the server running.

Ultimately I believe it's wrong by the HTTP spec to return a body in this case, and some clients will handle it and some wont because it's not well defined what they should do in this situation. But whether http4s should override the explicitly stated intentions of the user (this isn't possible with the standard DSL) to enforce the correct behaviour is a bigger question