Share HTTP semantics between HTTP/1 and 2 (#329)

* Move Bandit.HTTP1.Adapter to Bandit.Adapter (no changes yet)

* Refactor generic adapter out of HTTP1

* Move upgrade policy support into transport

* Factor common HTTP config options to their own config stanza

* First pass at pushing transport_info into the transport

* Add back @for declarations for Socket protocol conformance

* Misc tidy on HTTP1

* Refactor HTTP2 to use generic adapter

* Remove separate 'websocket_enabled' flag and appeal directly to opts

* Remove h2c upgrade support via Upgrade header

* Remove request_target, method, status from telemetry

* Simply pipeline API

* Update CHANGELOG & docs

CHANGELOG.md

@@ -5,32 +5,45 @@
 * Complete refactor of HTTP/2. Improved process model is MUCH easier to
  understand and yields about a 10% performance boost to HTTP/2 requests (#286 /
  #307)
+* Substantial refactor of the HTTP/1 and HTTP/2 stacks to share a common code
+ path for much of their implementations, with the protocol-specific parts being
+ factored out to a minimal `Bandit.HTTPTransport` protocol internally, which
+ allows each protocol to define its own implementation for the minimal set of
+ things that are different between the two stacks (#297 / #329)
 
 ### Changes
 
-* Remove `req_line_bytes`, `req_header_bytes`, `resp_line_bytes` and
- `resp_header_bytes` from HTTP/1 request telemetry
+* **BREAKING CHANGE** Move configuration options that are common between HTTP/1
+ and HTTP/2 stacks into a shared `http_options` top-level config
+* **BREAKING CHANGE** The HTTP/2 header size limit options have been deprecated,
+ and have been replaced with a single `max_header_block_size` option. The setting
+ defaults to 50k bytes, and refers to the size of the compressed header block
+ as sent on the wire (including any continuation frames)
+* **BREAKING CHANGE** Remove `req_line_bytes`, `req_header_bytes`, `resp_line_bytes` and
+ `resp_header_bytes` from HTTP/1 request telemetry measurements
+* **BREAKING CHANGE** Remove `status`, `method` and `request_target` from
+ telemetry metadata. All of this information can be obtained from the `conn`
+ struct attached to most telemetry events
+* **BREAKING CHANGE** Re-reading a body that has already been read returns `{:ok,
+ "", conn}` instead of raising a `Bandit.BodyAlreadyReadError`
+* **BREAKING CHANGE** Remove `Bandit.BodyAlreadyReadError`
+* **BREAKING CHANGE** Remove h2c support via Upgrade header. This was deprecated
+ in RFC9113 and never in widespread use. We continue to support h2c via prior
+ knowledge, which remains the only supported mechanism for h2c in RFC9113
 * Treat trailing bytes beyond the indicated content-length on HTTP/1 requests as
  an error
 * Surface request body read timeouts on HTTP/1 requests as `{:more...}` tuples
  and not errors
-* Re-reading a body on HTTP/1 requests that has already been read returns `{:ok,
- "", conn}` instead of raising a `Bandit.BodyAlreadyReadError`
-* Remove `Bandit.BodyAlreadyReadError`
 * Socket sending errors are no longer surfaced on chunk sends in HTTP/1
-* **BREAKING CHANGE** The HTTP/2 header size limit options have been deprecated,
- and have been replaced with a single `max_header_block_size` option. The setting
- defaults to 50k bytes, and refers to the size of the compressed header block
- as sent on the wire (including any continuation frames)
 * We no longer log if processes that are linked to an HTTP/2 stream process
  terminate unexpectedly. This has always been unspecified behaviour so is not
  considered a breaking change
 * Calls of `Plug.Conn` functions for an HTTP/2 connection must now come from the
  stream process; any other process will raise an error. Again, this has always
  been unspecified behaviour
-* Reading the body of an HTTP/2 request after it has already been read will
- return `{:ok, ""}` instead of raising a `Bandit.BodyAlreadyReadError` as it
- previously did
+* We now send an empty DATA frame for explicitly zero byte bodies instead of
+ optimizing to a HEADERS frame with end_stream set (we still do so for cases
+ such as 204/304 and HEAD requests)
 * We now send RST_STREAM frames if we complete a stream and the remote end is
  still open. This optimizes cases where the client may still be sending a body
  that we never consumed and don't care about

README.md

@@ -57,9 +57,7 @@ foundational work that is approachable & understandable by users above it in the
 * Complete server support for HTTP/2 as defined in [RFC
  9113](https://datatracker.ietf.org/doc/html/rfc9113) & [RFC
  9110](https://datatracker.ietf.org/doc/html/rfc9110), comprehensively covered
- by automated [h2spec](https://github.com/summerwind/h2spec) conformance
- testing. Though deprecated by later RFCs, Bandit also supports h2c upgrades as
- specified in [RFC7540](https://datatracker.ietf.org/doc/html/rfc7540)
+ by automated [h2spec](https://github.com/summerwind/h2spec) conformance testing
 * Support for HTTP content encoding compression on both HTTP/1.x and HTTP/2.
  gzip and deflate methods are supported per
  [RFC9110§8.4.1.{2,3}](https://www.rfc-editor.org/rfc/rfc9110.html#section-8.4.1.2)

lib/bandit.ex

@@ -58,6 +58,8 @@ defmodule Bandit do
  default values in this list based on your top-level configuration; these values will be
  overridden by values appearing here. A complete list can be found at
  `t:ThousandIsland.options/0`
+ * `http_options`: A list of options to configure the shared aspects of Bandit's HTTP stack. A
+ complete list can be found at `t:http_options/0`
  * `http_1_options`: A list of options to configure Bandit's HTTP/1 stack. A complete list can
  be found at `t:http_1_options/0`
  * `http_2_options`: A list of options to configure Bandit's HTTP/2 stack. A complete list can
@@ -77,11 +79,26 @@ defmodule Bandit do
  display_plug: module(),
  startup_log: Logger.level() | false,
  thousand_island_options: ThousandIsland.options(),
+ http_options: http_options(),
  http_1_options: http_1_options(),
  http_2_options: http_2_options(),
  websocket_options: websocket_options()
  ]
 
+ @typedoc """
+ Options to configure shared aspects of the HTTP stack in Bandit
+
+ * `compress`: Whether or not to attempt compression of responses via content-encoding
+ negotiation as described in
+ [RFC9110§8.4](https://www.rfc-editor.org/rfc/rfc9110.html#section-8.4). Defaults to true
+ * `deflate_options`: A keyword list of options to set on the deflate library. A complete list can
+ be found at `t:deflate_options/0`
+ """
+ @type http_options :: [
+ compress: boolean(),
+ deflate_opions: deflate_options()
+ ]
+
  @typedoc """
  Options to configure the HTTP/1 stack in Bandit
 
@@ -101,11 +118,6 @@ defmodule Bandit do
  Defaults to `false`
  * `log_protocol_errors`: Whether or not to log protocol errors such as malformed requests.
  Defaults to `true`
- * `compress`: Whether or not to attempt compression of responses via content-encoding
- negotiation as described in
- [RFC9110§8.4](https://www.rfc-editor.org/rfc/rfc9110.html#section-8.4). Defaults to true
- * `deflate_options`: A keyword list of options to set on the deflate library. A complete list can
- be found at `t:deflate_options/0`
  """
  @type http_1_options :: [
  enabled: boolean(),
@@ -115,9 +127,7 @@ defmodule Bandit do
  max_requests: pos_integer(),
  gc_every_n_keepalive_requests: pos_integer(),
  log_unknown_messages: boolean(),
- log_protocol_errors: boolean(),
- compress: boolean(),
- deflate_opions: deflate_options()
+ log_protocol_errors: boolean()
  ]
 
  @typedoc """
@@ -131,19 +141,12 @@ defmodule Bandit do
  HTTP/2 connection before closing the connection. Defaults to 0 (no limit)
  * `default_local_settings`: Options to override the default values for local HTTP/2
  settings. Values provided here will override the defaults specified in RFC9113§6.5.2
- * `compress`: Whether or not to attempt compression of responses via content-encoding
- negotiation as described in
- [RFC9110§8.4](https://www.rfc-editor.org/rfc/rfc9110.html#section-8.4). Defaults to true
- * `deflate_options`: A keyword list of options to set on the deflate library. A complete list can
- be found at `t:deflate_options/0`
  """
  @type http_2_options :: [
  enabled: boolean(),
  max_header_block_size: pos_integer(),
  max_requests: pos_integer(),
- default_local_settings: Bandit.HTTP2.Settings.t(),
- compress: boolean(),
- deflate_options: deflate_options()
+ default_local_settings: Bandit.HTTP2.Settings.t()
  ]
 
  @typedoc """
@@ -192,9 +195,10 @@ defmodule Bandit do
  }
  end
 
- @top_level_keys ~w(plug scheme port ip keyfile certfile otp_app cipher_suite display_plug startup_log thousand_island_options http_1_options http_2_options websocket_options)a
- @http_1_keys ~w(enabled max_request_line_length max_header_length max_header_count max_requests gc_every_n_keepalive_requests log_unknown_messages log_protocol_errors compress deflate_options)a
- @http_2_keys ~w(enabled max_header_block_size max_requests default_local_settings compress deflate_options)a
+ @top_level_keys ~w(plug scheme port ip keyfile certfile otp_app cipher_suite display_plug startup_log thousand_island_options http_options http_1_options http_2_options websocket_options)a
+ @http_keys ~w(compress deflate_options)a
+ @http_1_keys ~w(enabled max_request_line_length max_header_length max_header_count max_requests gc_every_n_keepalive_requests log_unknown_messages log_protocol_errors)a
+ @http_2_keys ~w(enabled max_header_block_size max_requests default_local_settings)a
  @websocket_keys ~w(enabled max_frame_size validate_text_frames compress)a
  @thousand_island_keys ThousandIsland.ServerConfig.__struct__()
  |> Map.from_struct()
@@ -212,6 +216,10 @@ defmodule Bandit do
  Keyword.get(arg, :thousand_island_options, [])
  |> validate_options(@thousand_island_keys, :thousand_island_options)
 
+ http_options =
+ Keyword.get(arg, :http_options, [])
+ |> validate_options(@http_keys, :http_options)
+
  http_1_options =
  Keyword.get(arg, :http_1_options, [])
  |> validate_options(@http_1_keys, :http_1_options)
@@ -230,15 +238,18 @@ defmodule Bandit do
 
  {http_1_enabled, http_1_options} = Keyword.pop(http_1_options, :enabled, true)
  {http_2_enabled, http_2_options} = Keyword.pop(http_2_options, :enabled, true)
- {websocket_enabled, websocket_options} = Keyword.pop(websocket_options, :enabled, true)
 
  handler_options = %{
  plug: plug,
  handler_module: Bandit.InitialHandler,
- opts: %{http_1: http_1_options, http_2: http_2_options, websocket: websocket_options},
+ opts: %{
+ http: http_options,
+ http_1: http_1_options,
+ http_2: http_2_options,
+ websocket: websocket_options
+ },
  http_1_enabled: http_1_enabled,
- http_2_enabled: http_2_enabled,
- websocket_enabled: websocket_enabled
+ http_2_enabled: http_2_enabled
  }
 
  scheme = Keyword.get(arg, :scheme, :http)

lib/bandit/http1/adapter.ex → lib/bandit/adapter.ex

@@ -1,51 +1,50 @@
-defmodule Bandit.HTTP1.Adapter do
+defmodule Bandit.Adapter do
  @moduledoc false
+ # Implements the Plug-facing `Plug.Conn.Adapter` behaviour. These functions provide the primary
+ # mechanism for Plug applications to interact with a client, including functions to read the
+ # client body (if sent) and send response information back to the client. The concerns in this
+ # module are broadly about the semantics of HTTP in general, and less about transport-specific
+ # concerns, which are managed by the underlying `Bandit.HTTPTransport` implementation
 
  @behaviour Plug.Conn.Adapter
 
- defstruct owner_pid: nil,
- transport: nil,
- transport_info: nil,
+ defstruct transport: nil,
+ owner_pid: nil,
  method: nil,
  status: nil,
  content_encoding: nil,
  upgrade: nil,
  metrics: %{},
- websocket_enabled: false,
  opts: []
 
  @typedoc "A struct for backing a Plug.Conn.Adapter"
  @type t :: %__MODULE__{
+ transport: Bandit.HTTPTransport.t(),
  owner_pid: pid() | nil,
- transport: Bandit.HTTPTransport.transport(),
- transport_info: Bandit.TransportInfo.t(),
  method: Plug.Conn.method() | nil,
  status: Plug.Conn.status() | nil,
  content_encoding: String.t(),
  upgrade: nil | {:websocket, opts :: keyword(), websocket_opts :: keyword()},
  metrics: %{},
- websocket_enabled: boolean(),
  opts: %{
- required(:http_1) => Bandit.http_1_options(),
+ required(:http) => Bandit.http_options(),
  required(:websocket) => Bandit.websocket_options()
  }
  }
 
- def init(owner_pid, transport, transport_info, method, headers, websocket_enabled, opts) do
+ def init(owner_pid, transport, method, headers, opts) do
  content_encoding =
  Bandit.Compression.negotiate_content_encoding(
  Bandit.Headers.get_header(headers, "accept-encoding"),
- Keyword.get(opts.http_1, :compress, true)
+ Keyword.get(opts.http, :compress, true)
  )
 
  %__MODULE__{
- owner_pid: owner_pid,
  transport: transport,
- transport_info: transport_info,
+ owner_pid: owner_pid,
  method: method,
  content_encoding: content_encoding,
  metrics: %{req_header_end_time: Bandit.Telemetry.monotonic_time()},
- websocket_enabled: websocket_enabled,
  opts: opts
  }
  end
@@ -58,7 +57,7 @@ defmodule Bandit.HTTP1.Adapter do
  adapter.metrics
  |> Map.put_new_lazy(:req_body_start_time, &Bandit.Telemetry.monotonic_time/0)
 
- case Bandit.HTTP1.Socket.read_data(adapter.transport, opts) do
+ case Bandit.HTTPTransport.read_data(adapter.transport, opts) do
  {:ok, body, transport} ->
  body = IO.iodata_to_binary(body)
 
@@ -115,7 +114,7 @@ defmodule Bandit.HTTP1.Adapter do
  resp_compression_method: content_encoding
  }
 
- deflate_options = Keyword.get(adapter.opts.http_1, :deflate_options, [])
+ deflate_options = Keyword.get(adapter.opts.http, :deflate_options, [])
  deflated_body = Bandit.Compression.compress(body, content_encoding, deflate_options)
  headers = [{"content-encoding", adapter.content_encoding} | headers]
  {deflated_body, headers, metrics}
@@ -124,7 +123,7 @@ defmodule Bandit.HTTP1.Adapter do
  {body, headers, %{}}
  end
 
- compress = Keyword.get(adapter.opts.http_1, :compress, true)
+ compress = Keyword.get(adapter.opts.http, :compress, true)
  headers = if compress, do: [{"vary", "accept-encoding"} | headers], else: headers
  headers = Bandit.Headers.add_content_length(headers, IO.iodata_length(body), status)
 
@@ -161,7 +160,7 @@ defmodule Bandit.HTTP1.Adapter do
 
  {socket, bytes_actually_written} =
  if send_resp_body?(adapter),
- do: {Bandit.HTTP1.Socket.sendfile(adapter.transport, path, offset, length), length},
+ do: {Bandit.HTTPTransport.sendfile(adapter.transport, path, offset, length), length},
  else: {adapter.transport, 0}
 
  metrics =
@@ -187,6 +186,12 @@ defmodule Bandit.HTTP1.Adapter do
 
  @impl Plug.Conn.Adapter
  def chunk(%__MODULE__{} = adapter, chunk) do
+ # Sending an empty chunk implicitly ends the response. This is a bit of an undefined corner of
+ # the Plug.Conn.Adapter behaviour (see https://github.com/elixir-plug/plug/pull/535 for
+ # details) and ending the response here carves closest to the underlying HTTP/1.1 behaviour
+ # (RFC9112§7.1). Since there is no notion of chunked encoding is in HTTP/2 anyway (RFC9113§8.1)
+ # this entire section of the API is a bit slanty regardless.
+
  validate_calling_process!(adapter)
  {:ok, nil, send_data(adapter, chunk, IO.iodata_length(chunk) == 0)}
  end
@@ -217,15 +222,15 @@ defmodule Bandit.HTTP1.Adapter do
  body_disposition = if send_resp_body?(adapter), do: body_disposition, else: :no_body
 
  socket =
- Bandit.HTTP1.Socket.send_headers(adapter.transport, status, headers, body_disposition)
+ Bandit.HTTPTransport.send_headers(adapter.transport, status, headers, body_disposition)
 
  %{adapter | transport: socket}
  end
 
  defp send_data(adapter, data, end_request) do
  socket =
  if send_resp_body?(adapter),
- do: Bandit.HTTP1.Socket.send_data(adapter.transport, data, end_request),
+ do: Bandit.HTTPTransport.send_data(adapter.transport, data, end_request),
  else: adapter.transport
 
  data_size = IO.iodata_length(data)
@@ -245,21 +250,27 @@ defmodule Bandit.HTTP1.Adapter do
  defp send_resp_body?(_adapter), do: true
 
  @impl Plug.Conn.Adapter
- def upgrade(%__MODULE__{websocket_enabled: true} = adapter, :websocket, opts),
- do: {:ok, %{adapter | upgrade: {:websocket, opts, adapter.opts.websocket}}}
-
- def upgrade(_adapter, _upgrade, _opts), do: {:error, :not_supported}
+ def upgrade(%__MODULE__{} = adapter, protocol, opts) do
+ if Keyword.get(adapter.opts.websocket, :enabled, true) &&
+ Bandit.HTTPTransport.supported_upgrade?(adapter.transport, protocol),
+ do: {:ok, %{adapter | upgrade: {protocol, opts, adapter.opts.websocket}}},
+ else: {:error, :not_supported}
+ end
 
  @impl Plug.Conn.Adapter
  def push(_adapter, _path, _headers), do: {:error, :not_supported}
 
  @impl Plug.Conn.Adapter
- def get_peer_data(%__MODULE__{transport_info: transport_info}),
- do: Bandit.TransportInfo.peer_data(transport_info)
+ def get_peer_data(%__MODULE__{} = adapter) do
+ case Bandit.HTTPTransport.transport_info(adapter.transport) do
+ {:ok, transport_info} -> Bandit.TransportInfo.peer_data(transport_info)
+ {:error, reason} -> raise "Unable to obtain transport_info: #{inspect(reason)}"
+ end
+ end
 
  @impl Plug.Conn.Adapter
  def get_http_protocol(%__MODULE__{} = adapter),
- do: Bandit.HTTP1.Socket.version(adapter.transport)
+ do: Bandit.HTTPTransport.version(adapter.transport)
 
  defp validate_calling_process!(%{owner_pid: owner}) when owner == self(), do: :ok
  defp validate_calling_process!(_), do: raise("Adapter functions must be called by stream owner")

lib/bandit/headers.ex

@@ -55,23 +55,6 @@ defmodule Bandit.Headers do
  end
  end
 
- @spec get_connection_header_keys(Plug.Conn.headers()) ::
- {:ok, [String.t()]} | {:error, String.t()}
- def get_connection_header_keys(headers) do
- case Bandit.Headers.get_header(headers, "connection") do
- nil ->
- {:error, "Expected connection header"}
-
- value ->
- header_keys =
- value
- |> String.downcase()
- |> Plug.Conn.Utils.list()
-
- {:ok, header_keys}
- end
- end
-
  @spec parse_content_length(binary()) :: {:ok, non_neg_integer()} | {:error, String.t()}
  defp parse_content_length(value) do
  case parse_integer(value) do