Better handle explicit chunked encoding responses

[Blacksmoke16]

So I know as per #5004 (comment) that Crystal natively handles chunked responses for you. However in Athena land, I have the two concepts separated into ATH::Response and ATH::StreamedResponse to make it more explicit. The latter of which sets up the response with transfer-encoding: chunked. However HTTP::Server doesn't seem to like this in some contexts:

server = HTTP::Server.new do |context|
  context.response.content_type = "application/json; charset=utf-8"
  context.response.headers["transfer-encoding"] = "chunked"
  context.response << "Hello world!"
end

Results in:

HTTP/1.1 200 OK
Connection: keep-alive
Content-Type: application/json; charset=utf-8
transfer-encoding: chunked
Content-Length: 12

curl: (56) chunk hex-length char not a hex digit: 0x48

As a follow up to #10353, I propose that we make two small changes to better support this use case:

1. Do not set content-length if there is a transfer-encoding header as per the RFC:

A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field.

2. Explicitly set @chunked = true if there is a transfer-encoding: chunked header

The diff for this would be something along the lines of:

diff --git a/spec/std/http/server/response_spec.cr b/spec/std/http/server/response_spec.cr
index 99e462151..173f2f9fd 100644
--- a/spec/std/http/server/response_spec.cr
+++ b/spec/std/http/server/response_spec.cr
@@ -76,6 +76,15 @@ describe HTTP::Server::Response do
     io.to_s.should eq("HTTP/1.1 304 Not Modified\r\nContent-Length: 5\r\n\r\n")
   end
 
+  it "allow explicitly configuring a `transfer-encoding` header" do
+    io = IO::Memory.new
+    response = Response.new(io)
+    response.headers["transfer-encoding"] = "chunked"
+    response.print "Hello"
+    response.close
+    io.to_s.should eq("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n5\r\nHello\r\n0\r\n\r\n")
+  end
+
   it "prints less then buffer's size" do
     io = IO::Memory.new
     response = Response.new(io)
diff --git a/src/http/server/response.cr b/src/http/server/response.cr
index 5c80b31cc..45f88e61e 100644
--- a/src/http/server/response.cr
+++ b/src/http/server/response.cr
@@ -255,11 +255,13 @@ class HTTP::Server
       private def unbuffered_write(slice : Bytes) : Nil
         return if slice.empty?
 
-        unless response.wrote_headers?
+        if !response.wrote_headers?
           if response.version != "HTTP/1.0" && !response.headers.has_key?("Content-Length")
             response.headers["Transfer-Encoding"] = "chunked"
             @chunked = true
           end
+        elsif response.headers.includes_word?("transfer-encoding", "chunked")
+          @chunked = true
         end
 
         ensure_headers_written
@@ -289,7 +291,7 @@ class HTTP::Server
         status = response.status
         set_content_length = !(status.not_modified? || status.no_content? || status.informational?)
 
-        if !response.wrote_headers? && !response.headers.has_key?("Content-Length") && set_content_length
+        if !response.wrote_headers? && !response.headers.has_key?("transfer-encoding") && !response.headers.has_key?("Content-Length") && set_content_length
           response.content_length = @out_count
         end

[straight-shoota]

The latter of which sets up the response with transfer-encoding: chunked.

I'm wondering if this is actually a good thing. Whoever sets the header should be considered responsible for implementing it. But that's not possible for the caller because HTTP::Server::Response handles chunked transfer encoding completely internally with no means for external influence. There's not really anything you can do about it, except for sending content in a way explicitly triggers it (or not).
However, setting the transfer-encoding header could serve as an alternative trigger. It would be more explicit than a random flush.

[ysbaddaden]

Explicitly set @chunked = true if there is a transfer-encoding header

This is wrong. The transfer-encoding header may have other values. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding

[straight-shoota]

Yeah, correct wording must be this:

Explicitly set @chunked = true if transfer-encoding header is set to chunked

[Blacksmoke16]

Thanks, good call. I updated the OP.

However, setting the transfer-encoding header could serve as an alternative trigger. It would be more explicit than a random flush.

Yea exactly. Esp given in Athena land I don't expose the HTTP::Server::Response so I need some other mechanism to influence how the response is written. I guess the other option would be don't change anything, but document ATH::StreamedResponse to be more explicit in how it needs to be used to avoid this error? 🤷.