Fix handling of HEAD responses (#242)

* Record HTTP/1 method in req

* Support HEAD, 204, 304 quelling for HTTP/1 responses

* Refactor h2 send_file logic

Also fix file always being closed after h2 sendfile file call, as in mtrudel/thousand_island#78

* Record HTTP/2 method in req

* Add content-length header to HTTP/2 sendfile responses

* Support HEAD, 204, 304 quelling for HTTP/2 responses

lib/bandit/http1/adapter.ex

@@ -14,6 +14,7 @@ defmodule Bandit.HTTP1.Adapter do
             body_remaining: nil,
             body_encoding: nil,
             version: nil,
+            method: nil,
             keepalive: false,
             content_encoding: nil,
             upgrade: nil,
@@ -31,6 +32,7 @@ defmodule Bandit.HTTP1.Adapter do
           body_remaining: nil | integer(),
           body_encoding: nil | binary(),
           version: nil | :"HTTP/1.1" | :"HTTP/1.0",
+          method: Plug.Conn.method() | nil,
           keepalive: boolean(),
           content_encoding: String.t(),
           upgrade: nil | {:websocket, opts :: keyword(), websocket_opts :: keyword()},
@@ -46,22 +48,23 @@ defmodule Bandit.HTTP1.Adapter do
   # Header Reading
   ################
 
-  def read_request_line(req, method \\ nil, request_target \\ nil) do
+  def read_request_line(req, request_target \\ nil) do
     packet_size = Keyword.get(req.opts.http_1, :max_request_line_length, 10_000)
 
     case :erlang.decode_packet(:http_bin, req.buffer, packet_size: packet_size) do
       {:more, _len} ->
         with {:ok, chunk} <- read_available(req.socket, _read_timeout = nil) do
-          read_request_line(%{req | buffer: req.buffer <> chunk}, method, request_target)
+          read_request_line(%{req | buffer: req.buffer <> chunk}, request_target)
         end
 
       {:ok, {:http_request, method, request_target, version}, rest} ->
         with {:ok, version} <- get_version(version),
              {:ok, request_target} <- resolve_request_target(request_target) do
+          method = to_string(method)
           bytes_read = byte_size(req.buffer) - byte_size(rest)
           metrics = Map.update(req.metrics, :req_line_bytes, bytes_read, &(&1 + bytes_read))
-          req = %{req | buffer: rest, version: version, metrics: metrics}
-          {:ok, to_string(method), request_target, req}
+          req = %{req | buffer: rest, version: version, method: method, metrics: metrics}
+          {:ok, request_target, req}
         end
 
       {:ok, {:http_error, reason}, _rest} ->
@@ -360,7 +363,7 @@ defmodule Bandit.HTTP1.Adapter do
   def send_resp(%__MODULE__{write_state: :chunking_out}, _, _, _),
     do: raise(Plug.Conn.AlreadySentError)
 
-  def send_resp(%__MODULE__{socket: socket, version: version} = req, status, headers, body) do
+  def send_resp(%__MODULE__{} = req, status, headers, body) do
     start_time = Bandit.Telemetry.monotonic_time()
     response_content_encoding_header = Bandit.Headers.get_header(headers, "content-encoding")
 
@@ -403,14 +406,22 @@ defmodule Bandit.HTTP1.Adapter do
     body_bytes = IO.iodata_length(body)
     headers = Bandit.Headers.add_content_length(headers, body_bytes, status)
 
-    {header_iodata, header_metrics} = response_header(version, status, headers)
-    _ = ThousandIsland.Socket.send(socket, [header_iodata, body])
+    {header_iodata, header_metrics} = response_header(req.version, status, headers)
+
+    {body_iodata, body_metrics} =
+      if send_resp_body?(req, status) do
+        {body, %{resp_body_bytes: body_bytes}}
+      else
+        {[], %{resp_body_bytes: 0}}
+      end
+
+    _ = ThousandIsland.Socket.send(req.socket, [header_iodata | body_iodata])
 
     metrics =
       req.metrics
       |> Map.merge(compression_metrics)
       |> Map.merge(header_metrics)
-      |> Map.put(:resp_body_bytes, body_bytes)
+      |> Map.merge(body_metrics)
       |> Map.put(:resp_start_time, start_time)
       |> Map.put(:resp_end_time, Bandit.Telemetry.monotonic_time())
 
@@ -436,15 +447,22 @@ defmodule Bandit.HTTP1.Adapter do
       end
 
     if offset + length <= size do
-      headers = [{"content-length", length |> to_string()} | headers]
+      headers = Bandit.Headers.add_content_length(headers, length, status)
       {header_iodata, header_metrics} = response_header(version, status, headers)
       _ = ThousandIsland.Socket.send(socket, header_iodata)
-      _ = ThousandIsland.Socket.sendfile(socket, path, offset, length)
+
+      body_metrics =
+        if send_resp_body?(req, status) do
+          _ = ThousandIsland.Socket.sendfile(socket, path, offset, length)
+          %{resp_body_bytes: length}
+        else
+          %{resp_body_bytes: 0}
+        end
 
       metrics =
         req.metrics
         |> Map.merge(header_metrics)
-        |> Map.put(:resp_body_bytes, length)
+        |> Map.merge(body_metrics)
         |> Map.put(:resp_start_time, start_time)
         |> Map.put(:resp_end_time, Bandit.Telemetry.monotonic_time())
 
@@ -469,15 +487,21 @@ defmodule Bandit.HTTP1.Adapter do
       |> Map.put(:resp_start_time, start_time)
       |> Map.put(:resp_body_bytes, 0)
 
-    {:ok, nil, %{req | write_state: :chunking_out, metrics: metrics}}
+    if send_resp_body?(req, status) do
+      {:ok, nil, %{req | write_state: :chunking_out, metrics: metrics}}
+    else
+      {:ok, nil, %{req | write_state: :sent, metrics: metrics}}
+    end
   end
 
   @impl Plug.Conn.Adapter
-  def chunk(%__MODULE__{socket: socket}, chunk) do
+  def chunk(%__MODULE__{write_state: :chunking_out, socket: socket}, chunk) do
     byte_size = chunk |> IO.iodata_length() |> Integer.to_string(16)
     ThousandIsland.Socket.send(socket, [byte_size, "\r\n", chunk, "\r\n"])
   end
 
+  def chunk(_, _), do: :ok
+
   @impl Plug.Conn.Adapter
   def inform(%__MODULE__{version: :"HTTP/1.0"}, _status, _headers), do: {:error, :not_supported}
 
@@ -525,6 +549,11 @@ defmodule Bandit.HTTP1.Adapter do
     {[resp_line, headers], metrics}
   end
 
+  defp send_resp_body?(%{method: "HEAD"}, _status), do: false
+  defp send_resp_body?(_req, 204), do: false
+  defp send_resp_body?(_req, 304), do: false
+  defp send_resp_body?(_req, _status), do: true
+
   @impl Plug.Conn.Adapter
   def upgrade(%Bandit.HTTP1.Adapter{websocket_enabled: true} = req, :websocket, opts),
     do: {:ok, %{req | upgrade: {:websocket, opts, req.opts.websocket}}}

lib/bandit/http1/handler.ex

@@ -22,24 +22,24 @@ defmodule Bandit.HTTP1.Handler do
 
     with {:ok, transport_info} <- Bandit.TransportInfo.init(socket),
          req <- %{req | transport_info: transport_info},
-         {:ok, method, request_target, req} <- Bandit.HTTP1.Adapter.read_request_line(req) do
+         {:ok, request_target, req} <- Bandit.HTTP1.Adapter.read_request_line(req) do
       try do
         with {:ok, headers, req} <- Bandit.HTTP1.Adapter.read_headers(req),
              {:ok, :no_upgrade} <-
-               maybe_upgrade_h2c(state, req, transport_info, method, request_target, headers),
+               maybe_upgrade_h2c(state, req, transport_info, req.method, request_target, headers),
              {:ok, %Plug.Conn{adapter: {Bandit.HTTP1.Adapter, req}} = conn} <-
                Bandit.Pipeline.run(
                  {Bandit.HTTP1.Adapter, req},
                  transport_info,
-                 method,
+                 req.method,
                  request_target,
                  headers,
                  state.plug
                ) do
           Bandit.Telemetry.stop_span(span, req.metrics, %{
             conn: conn,
             status: conn.status,
-            method: method,
+            method: req.method,
             request_target: request_target
           })
 
@@ -52,7 +52,7 @@ defmodule Bandit.HTTP1.Handler do
             Bandit.Telemetry.stop_span(span, %{}, %{
               error: reason,
               status: code,
-              method: method,
+              method: req.method,
               request_target: request_target
             })
 
@@ -62,7 +62,7 @@ defmodule Bandit.HTTP1.Handler do
             Bandit.Telemetry.stop_span(span, req.metrics, %{
               conn: conn,
               status: conn.status,
-              method: method,
+              method: req.method,
               request_target: request_target
             })
 

lib/bandit/http2/adapter.ex

@@ -10,6 +10,7 @@ defmodule Bandit.HTTP2.Adapter do
             transport_info: nil,
             stream_id: nil,
             end_stream: false,
+            method: nil,
             content_encoding: nil,
             metrics: %{},
             opts: []
@@ -20,6 +21,7 @@ defmodule Bandit.HTTP2.Adapter do
           transport_info: Bandit.TransportInfo.t(),
           stream_id: Bandit.HTTP2.Stream.stream_id(),
           end_stream: boolean(),
+          method: Plug.Conn.method() | nil,
           content_encoding: String.t() | nil,
           metrics: map(),
           opts: keyword()
@@ -147,7 +149,7 @@ defmodule Bandit.HTTP2.Adapter do
     headers = Bandit.Headers.add_content_length(headers, body_bytes, status)
 
     adapter =
-      if body_bytes == 0 do
+      if body_bytes == 0 || !send_resp_body?(adapter, status) do
         adapter
         |> send_headers(status, headers, true)
       else
@@ -168,46 +170,54 @@ defmodule Bandit.HTTP2.Adapter do
   def send_file(%__MODULE__{} = adapter, status, headers, path, offset, length) do
     %File.Stat{type: :regular, size: size} = File.stat!(path)
     length = if length == :all, do: size - offset, else: length
+    headers = Bandit.Headers.add_content_length(headers, length, status)
 
-    cond do
-      offset + length == size && offset == 0 ->
-        adapter = send_headers(adapter, status, headers, false)
+    adapter =
+      cond do
+        !send_resp_body?(adapter, status) ->
+          send_headers(adapter, status, headers, true)
+
+        offset + length == size && offset == 0 ->
+          adapter = send_headers(adapter, status, headers, false)
 
-        adapter =
           path
           |> File.stream!([], 2048)
           |> Enum.reduce(adapter, fn chunk, adapter -> send_data(adapter, chunk, false) end)
           |> send_data(<<>>, true)
 
-        metrics =
-          adapter.metrics
-          |> Map.put(:resp_end_time, Bandit.Telemetry.monotonic_time())
-
-        {:ok, nil, %{adapter | metrics: metrics}}
-
-      offset + length < size ->
-        with {:ok, fd} <- :file.open(path, [:raw, :binary]),
-             {:ok, data} <- :file.pread(fd, offset, length) do
-          adapter =
-            adapter
-            |> send_headers(status, headers, false)
-            |> send_data(data, true)
-
-          metrics =
-            adapter.metrics
-            |> Map.put(:resp_end_time, Bandit.Telemetry.monotonic_time())
+        offset + length < size ->
+          case :file.open(path, [:raw, :binary]) do
+            {:ok, fd} ->
+              try do
+                with {:ok, data} <- :file.pread(fd, offset, length) do
+                  adapter
+                  |> send_headers(status, headers, false)
+                  |> send_data(data, true)
+                end
+              after
+                :file.close(fd)
+              end
+
+            {:error, reason} ->
+              {:error, reason}
+          end
+
+        true ->
+          raise "Cannot read #{length} bytes starting at #{offset} as #{path} is only #{size} octets in length"
+      end
 
-          {:ok, nil, %{adapter | metrics: metrics}}
-        end
+    metrics = Map.put(adapter.metrics, :resp_end_time, Bandit.Telemetry.monotonic_time())
 
-      true ->
-        raise "Cannot read #{length} bytes starting at #{offset} as #{path} is only #{size} octets in length"
-    end
+    {:ok, nil, %{adapter | metrics: metrics}}
   end
 
   @impl Plug.Conn.Adapter
   def send_chunked(%__MODULE__{} = adapter, status, headers) do
-    {:ok, nil, send_headers(adapter, status, headers, false)}
+    if send_resp_body?(adapter, status) do
+      {:ok, nil, send_headers(adapter, status, headers, false)}
+    else
+      {:ok, nil, send_headers(adapter, status, headers, true)}
+    end
   end
 
   @impl Plug.Conn.Adapter
@@ -217,6 +227,10 @@ defmodule Bandit.HTTP2.Adapter do
     # details) and closing the stream here carves closest to the underlying HTTP/1.1 behaviour
     # (RFC9112§7.1). The whole notion of chunked encoding is moot in HTTP/2 anyway (RFC9113§8.1)
     # so this entire section of the API is a bit slanty regardless.
+    #
+    # Moreover, if the caller is chunking out on a HEAD, 204 or 304 response, the underlying
+    # stream will have been closed in send_chunked/3 above, and so this call will return an
+    # `{:error, :not_owner}` error here (which we ignore, but it's still kinda odd)
     _ = send_data(adapter, chunk, IO.iodata_length(chunk) == 0)
     :ok
   end
@@ -242,6 +256,11 @@ defmodule Bandit.HTTP2.Adapter do
   @impl Plug.Conn.Adapter
   def get_http_protocol(%__MODULE__{}), do: :"HTTP/2"
 
+  defp send_resp_body?(%{method: "HEAD"}, _status), do: false
+  defp send_resp_body?(_req, 204), do: false
+  defp send_resp_body?(_req, 304), do: false
+  defp send_resp_body?(_req, _status), do: true
+
   defp send_headers(adapter, status, headers, end_stream) do
     metrics =
       adapter.metrics

lib/bandit/http2/stream_task.ex

@@ -52,7 +52,8 @@ defmodule Bandit.HTTP2.StreamTask do
 
   def run(req, transport_info, all_headers, plug, span) do
     with {:ok, request_target} <- build_request_target(all_headers),
-         method <- Bandit.Headers.get_header(all_headers, ":method") do
+         method <- Bandit.Headers.get_header(all_headers, ":method"),
+         req <- %{req | method: method} do
       with {:ok, pseudo_headers, headers} <- split_headers(all_headers),
            :ok <- pseudo_headers_all_request(pseudo_headers),
            :ok <- exactly_one_instance_of(pseudo_headers, ":scheme"),