Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove HTTP/2 Push Promise support since Plug is deprecating it #33

Merged
merged 5 commits into from
Oct 4, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions lib/bandit/http2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ The details of a particular stream are contained within a `Bandit.HTTP2.Stream`
`Bandit.HTTP2.StreamCollection` module manages a collection of streams, allowing for the memory
efficient management of complete & yet unborn streams alongside active ones.

Once a complete header block has been read (or, in the case of push promises, once a push promise
request has been processed), a `Bandit.HTTP2.StreamTask` is started to manage the actual calling
of the configured `Plug` module for this server, using the `Bandit.HTTP2.Adapter` module as the
implementation of the `Plug.Conn.Adapter` behaviour. This adapter uses a simple `receive` pattern to
listen for messages sent to it from the connection process, a pattern chosen because it allows for
easy provision of the blocking-style API required by the `Plug.Conn.Adapter` behaviour. Functions
in the `Bandit.HTTP2.Adapter` behaviour which write data to the client use `GenServer` calls to
the `Bandit.HTTP2.Handler` module in order to pass data to the connection process.
Once a complete header block has been read, a `Bandit.HTTP2.StreamTask` is started to manage the
actual calling of the configured `Plug` module for this server, using the `Bandit.HTTP2.Adapter`
module as the implementation of the `Plug.Conn.Adapter` behaviour. This adapter uses a simple
`receive` pattern to listen for messages sent to it from the connection process, a pattern chosen
because it allows for easy provision of the blocking-style API required by the `Plug.Conn.Adapter`
behaviour. Functions in the `Bandit.HTTP2.Adapter` behaviour which write data to the client use
`GenServer` calls to the `Bandit.HTTP2.Handler` module in order to pass data to the connection
process.

# Testing

Expand Down
10 changes: 1 addition & 9 deletions lib/bandit/http2/adapter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,7 @@ defmodule Bandit.HTTP2.Adapter do
end

@impl Plug.Conn.Adapter
def push(adapter, path, headers) do
headers = split_cookies(headers)
headers = [{":path", path} | headers]
headers = [{":authority", adapter.uri.authority} | headers]
headers = [{":scheme", adapter.uri.scheme} | headers]
headers = [{":method", "GET"} | headers]

GenServer.call(adapter.connection, {:send_push, adapter.stream_id, headers})
end
def push(_adapter, _path, _headers), do: {:error, :not_supported}

@impl Plug.Conn.Adapter
def get_peer_data(%__MODULE__{peer: peer}), do: peer
Expand Down
40 changes: 0 additions & 40 deletions lib/bandit/http2/connection.ex
Original file line number Diff line number Diff line change
Expand Up @@ -283,15 +283,6 @@ defmodule Bandit.HTTP2.Connection do
end
end

def handle_frame(%Frame.PushPromise{}, socket, connection) do
shutdown_connection(
Errors.protocol_error(),
"Received PUSH_PROMISE (RFC7540§8.2)",
socket,
connection
)
end

# Catch-all handler for unknown frame types

def handle_frame(%Frame.Unknown{} = frame, _socket, connection) do
Expand Down Expand Up @@ -428,37 +419,6 @@ defmodule Bandit.HTTP2.Connection do
end
end

@spec send_push(Stream.stream_id(), Plug.Conn.headers(), Socket.t(), t()) ::
{:ok, t()} | {:error, term()}
def send_push(stream_id, headers, socket, connection) do
with :ok <- can_send_push_promises(connection),
:ok <- StreamCollection.can_send_new_push_promise(connection.streams),
promised_stream_id <- StreamCollection.next_local_stream_id(connection.streams),
{:ok, stream} <- StreamCollection.get_stream(connection.streams, promised_stream_id),
{:ok, stream} <- Stream.send_push_headers(stream, headers),
enc_headers <- Enum.map(headers, fn {key, value} -> {:store, key, value} end),
{block, send_hpack_state} <- HPAX.encode(enc_headers, connection.send_hpack_state),
:ok <- send_push_promise_frame(stream_id, promised_stream_id, block, socket, connection),
{:ok, stream} <- Stream.start_push(stream, headers, connection.peer, connection.plug),
{:ok, streams} <- StreamCollection.put_stream(connection.streams, stream) do
{:ok, %{connection | send_hpack_state: send_hpack_state, streams: streams}}
end
end

defp can_send_push_promises(%__MODULE__{remote_settings: %Settings{enable_push: true}}), do: :ok
defp can_send_push_promises(_), do: {:error, :client_disabled_push}

defp send_push_promise_frame(stream_id, promised_stream_id, block, socket, connection) do
%Frame.PushPromise{
stream_id: stream_id,
promised_stream_id: promised_stream_id,
fragment: block
}
|> send_frame(socket, connection)

:ok
end

@spec stream_terminated(pid(), term(), Socket.t(), t()) :: {:ok, t()} | {:error, term()}
def stream_terminated(pid, reason, socket, connection) do
with {:ok, stream} <- StreamCollection.get_active_stream_by_pid(connection.streams, pid),
Expand Down
1 change: 0 additions & 1 deletion lib/bandit/http2/frame.ex
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ defmodule Bandit.HTTP2.Frame do
| Frame.Priority.t()
| Frame.RstStream.t()
| Frame.Settings.t()
| Frame.PushPromise.t()
| Frame.Ping.t()
| Frame.Goaway.t()
| Frame.WindowUpdate.t()
Expand Down
95 changes: 3 additions & 92 deletions lib/bandit/http2/frame/push_promise.ex
Original file line number Diff line number Diff line change
@@ -1,100 +1,11 @@
defmodule Bandit.HTTP2.Frame.PushPromise do
@moduledoc false

import Bandit.HTTP2.Frame.Flags

alias Bandit.HTTP2.{Connection, Errors, Frame, Stream}

defstruct stream_id: nil,
end_headers: false,
promised_stream_id: nil,
fragment: nil

@typedoc "An HTTP/2 PUSH_PROMISE frame"
@type t :: %__MODULE__{
stream_id: Stream.stream_id(),
end_headers: boolean(),
promised_stream_id: Stream.stream_id(),
fragment: iodata()
}

@end_headers_bit 2
@padding_bit 3

@spec deserialize(Frame.flags(), Stream.stream_id(), iodata()) ::
{:ok, t()} | {:error, Connection.error()}
def deserialize(_flags, 0, _payload) do
{:error,
{:connection, Errors.protocol_error(),
"PUSH_PROMISE frame with zero stream_id (RFC7540§6.6)"}}
end

def deserialize(
flags,
stream_id,
<<padding_length::8, 0::1, promised_stream_id::31, rest::binary>>
)
when set?(flags, @padding_bit) and byte_size(rest) >= padding_length do
{:ok,
%__MODULE__{
stream_id: stream_id,
end_headers: set?(flags, @end_headers_bit),
promised_stream_id: promised_stream_id,
fragment: binary_part(rest, 0, byte_size(rest) - padding_length)
}}
end

def deserialize(flags, stream_id, <<0::1, promised_stream_id::31, fragment::binary>>)
when clear?(flags, @padding_bit) do
{:ok,
%__MODULE__{
stream_id: stream_id,
end_headers: set?(flags, @end_headers_bit),
promised_stream_id: promised_stream_id,
fragment: fragment
}}
end

def deserialize(
flags,
_stream_id,
<<_padding_length::8, _reserved::1, _promised_stream_id::31, _rest::binary>>
)
when set?(flags, @padding_bit) do
{:error,
{:connection, Errors.protocol_error(),
"PUSH_PROMISE frame with invalid padding length (RFC7540§6.6)"}}
end

defimpl Frame.Serializable do
alias Bandit.HTTP2.Frame.{Continuation, PushPromise}

@end_headers_bit 2

def serialize(%PushPromise{} = frame, max_frame_size) do
fragment_length = IO.iodata_length(frame.fragment)
max_fragment_size = max_frame_size - 4

if fragment_length <= max_fragment_size do
[
{0x5, set([@end_headers_bit]), frame.stream_id,
[<<frame.promised_stream_id::32>>, frame.fragment]}
]
else
<<this_frame::binary-size(max_fragment_size), rest::binary>> =
IO.iodata_to_binary(frame.fragment)

[
{0x5, 0x00, frame.stream_id, <<frame.promised_stream_id::32, this_frame::binary>>}
| Frame.Serializable.serialize(
%Continuation{
stream_id: frame.stream_id,
fragment: rest
},
max_frame_size
)
]
end
end
{:error, Connection.error()}
def deserialize(_flags, _stream, _payload) do
{:error, {:connection, Errors.protocol_error(), "PUSH_PROMISE frame received (RFC7540§8.2)"}}
end
end
6 changes: 2 additions & 4 deletions lib/bandit/http2/frame/settings.ex
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ defmodule Bandit.HTTP2.Frame.Settings do
{:cont, {:ok, %{acc | header_table_size: value}}}

{:ok, {0x02, 0x01}}, {:ok, acc} ->
{:cont, {:ok, %{acc | enable_push: true}}}
{:cont, {:ok, acc}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these cases all together (and ignore invalid enable_push values)?
h2spec seems fine with it.

Otherwise group the valid cases to {:ok, {0x02, val}}, {:ok, acc} when val in [0x00, 0x01] ->?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep the handling, even if h2spec doesn't complain. No sense in walking back strict conformance.

Updated to use a when clause, as suggested

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe use {:ok, {0x02, value}}, {:ok, _acc} when value not in [0x00, 0x01] on the error case and drop the noop case?


{:ok, {0x02, 0x00}}, {:ok, acc} ->
{:cont, {:ok, %{acc | enable_push: false}}}
{:cont, {:ok, acc}}

{:ok, {0x02, _value}}, {:ok, _acc} ->
{:halt, {:error, Errors.protocol_error(), "Invalid enable_push value (RFC7540§6.5)"}}
Expand Down Expand Up @@ -109,8 +109,6 @@ defmodule Bandit.HTTP2.Frame.Settings do
|> Enum.map(fn
{:header_table_size, 4_096} -> <<>>
{:header_table_size, value} -> <<0x01::16, value::32>>
{:enable_push, true} -> <<>>
{:enable_push, false} -> <<0x02::16, 0x00::32>>
{:max_concurrent_streams, :infinity} -> <<>>
{:max_concurrent_streams, value} -> <<0x03::16, value::32>>
{:initial_window_size, 65_535} -> <<>>
Expand Down
10 changes: 0 additions & 10 deletions lib/bandit/http2/handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,6 @@ defmodule Bandit.HTTP2.Handler do
end
end

def handle_call({:send_push, stream_id, headers}, _from, {socket, state}) do
case Connection.send_push(stream_id, headers, socket, state.connection) do
{:ok, connection} ->
{:reply, :ok, {socket, %{state | connection: connection}}}

{:error, reason} ->
{:reply, {:error, reason}, {socket, state}}
end
end

def handle_info({:EXIT, pid, reason}, {socket, state}) do
case Connection.stream_terminated(pid, reason, socket, state.connection) do
{:ok, connection} ->
Expand Down
2 changes: 0 additions & 2 deletions lib/bandit/http2/settings.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ defmodule Bandit.HTTP2.Settings do
# Settings as defined in RFC7540§6.5.2 and §11.3

defstruct header_table_size: 4_096,
enable_push: true,
max_concurrent_streams: :infinity,
initial_window_size: 65_535,
max_frame_size: 16_384,
Expand All @@ -12,7 +11,6 @@ defmodule Bandit.HTTP2.Settings do
@typedoc "A collection of settings as defined in RFC7540§6"
@type t :: %__MODULE__{
header_table_size: non_neg_integer(),
enable_push: boolean(),
max_concurrent_streams: non_neg_integer() | :infinity,
initial_window_size: non_neg_integer(),
max_frame_size: non_neg_integer(),
Expand Down
35 changes: 0 additions & 35 deletions lib/bandit/http2/stream.ex
Original file line number Diff line number Diff line change
Expand Up @@ -77,32 +77,6 @@ defmodule Bandit.HTTP2.Stream do
{:error, {:connection, Errors.protocol_error(), "Received HEADERS in unexpected state"}}
end

@spec send_push_headers(t(), Plug.Conn.headers()) :: {:ok, t()} | {:error, error}
def send_push_headers(%__MODULE__{state: :idle} = stream, headers) do
with :ok <- stream_id_is_valid_server(stream.stream_id),
:ok <- headers_all_lowercase(headers, stream.stream_id),
:ok <- pseudo_headers_all_request(headers, stream.stream_id),
:ok <- pseudo_headers_first(headers, stream.stream_id),
:ok <- no_connection_headers(headers, stream.stream_id),
:ok <- valid_te_header(headers, stream.stream_id),
:ok <- exactly_one_instance_of(headers, ":scheme", stream.stream_id),
:ok <- exactly_one_instance_of(headers, ":method", stream.stream_id),
:ok <- exactly_one_instance_of(headers, ":path", stream.stream_id),
:ok <- non_empty_path(headers, stream.stream_id) do
{:ok, %{stream | state: :reserved_local}}
else
{:error, {:connection, _error_code, error_message}} -> {:error, error_message}
{:error, {:stream, _stream_id, _error_code, error_message}} -> {:error, error_message}
end
end

@spec start_push(t(), Plug.Conn.headers(), Plug.Conn.Adapter.peer_data(), Bandit.plug()) ::
{:ok, t()}
def start_push(%__MODULE__{state: :reserved_local} = stream, headers, peer, plug) do
{:ok, pid} = StreamTask.start_link(self(), stream.stream_id, headers, peer, plug)
{:ok, %{stream | state: :remote_closed, pid: pid}}
end

# RFC7540§5.1.1 - client initiated streams must be odd
defp stream_id_is_valid_client(stream_id) do
if Integer.is_odd(stream_id) do
Expand All @@ -112,15 +86,6 @@ defmodule Bandit.HTTP2.Stream do
end
end

# RFC7540§5.1.1 - server initiated streams must be even
defp stream_id_is_valid_server(stream_id) do
if Integer.is_even(stream_id) do
:ok
else
{:error, {:connection, Errors.protocol_error(), "Sending HEADERS with odd stream_id"}}
end
end

# RFC7540§8.1.2 - all headers name fields must be lowercsae
defp headers_all_lowercase(headers, stream_id) do
headers
Expand Down
21 changes: 0 additions & 21 deletions lib/bandit/http2/stream_collection.ex
Original file line number Diff line number Diff line change
Expand Up @@ -121,27 +121,6 @@ defmodule Bandit.HTTP2.StreamCollection do
end
end

@spec can_send_new_push_promise(t()) :: :ok | {:error, :max_concurrent_streams}
def can_send_new_push_promise(collection) do
case collection.max_concurrent_streams do
:infinity ->
:ok

max_concurrent_streams ->
# Only count server-started (ie: push) streams per RFC7540§6.5.2
current_push_stream_count =
collection.streams
|> Map.values()
|> Enum.count(&(&1.state in [:open, :remote_closed] && Integer.is_even(&1.stream_id)))

if max_concurrent_streams > current_push_stream_count do
:ok
else
{:error, :max_concurrent_streams}
end
end
end

@spec next_local_stream_id(t()) :: Stream.stream_id()
def next_local_stream_id(collection), do: collection.last_local_stream_id + 2

Expand Down