Improve table resizing #19
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 30edd36f7534e73ee1e0eb62fd65de1e40a3be3e-PR-19Details
💛 - Coveralls |
|
@whatyouhide anything you need from me to move this along? I'm keen to get this merged and downstreamed into Bandit as it's blocking real use cases behind AWS load balancers. As I mention in #18, this shouldn't require anything beyond a patch release; there's no user-facing change in either functionality or behaviour. |
whatyouhide
reviewed
Dec 4, 2024
lib/hpax.ex
Outdated
Comment on lines
85
to
87
| Resizes the given table to the given maximum size. Intended for use where the | ||
| overlying protocol has signalled a change to the table's maximum size, | ||
| such as when an HTTP/2 Settings frame is received. |
There was a problem hiding this comment.
Suggested change
| Resizes the given table to the given maximum size. Intended for use where the | |
| overlying protocol has signalled a change to the table's maximum size, | |
| such as when an HTTP/2 Settings frame is received. | |
| Resizes the given table to the given maximum size. | |
| This is intended for use where the | |
| overlying protocol has signaled a change to the table's maximum size, | |
| such as when an HTTP/2 `SETTINGS` frame is received. |
lib/hpax.ex
Outdated
| If the indicated size is less than the table's current size, entries | ||
| will be evicted as needed to fit within the specified size, and the table's | ||
| maximum size will be decreased to the specified value. A flag will also be | ||
| set which will enqueue a 'dynamic table size update' command to be prefixed |
There was a problem hiding this comment.
Suggested change
| set which will enqueue a 'dynamic table size update' command to be prefixed | |
| set which will enqueue a "dynamic table size update" command to be prefixed |
lib/hpax.ex
Outdated
| will be evicted as needed to fit within the specified size, and the table's | ||
| maximum size will be decreased to the specified value. A flag will also be | ||
| set which will enqueue a 'dynamic table size update' command to be prefixed | ||
| to the next block encoded with this table, per RFC9113§4.3.1. |
There was a problem hiding this comment.
Can you link to that RFC section plz?
lib/hpax.ex
Outdated
| @@ -149,7 +160,9 @@ defmodule HPAX do | |||
| when header: {action, header_name(), header_value()}, | |||
| action: :store | :store_name | :no_store | :never_store | |||
| def encode(headers, %Table{} = table) when is_list(headers) do | |||
| encode_headers(headers, table, _acc = []) | |||
| {table, pending_resizes} = Table.pending_resizes(table) | |||
| acc = pending_resizes |> Enum.map(&[<<0b001::3, Types.encode_integer(&1, 5)::bitstring>>]) | |||
There was a problem hiding this comment.
Suggested change
| acc = pending_resizes |> Enum.map(&[<<0b001::3, Types.encode_integer(&1, 5)::bitstring>>]) | |
| acc = Enum.map(pending_resizes, &[<<0b001::3, Types.encode_integer(&1, 5)::bitstring>>]) |
lib/hpax/table.ex
Outdated
|
|
||
| If the existing entries do not fit in the new table size the oldest entries are evicted. | ||
| In all cases, the table's protocol_max_table_size is updated accordingly |
There was a problem hiding this comment.
Suggested change
| In all cases, the table's protocol_max_table_size is updated accordingly | |
| In all cases, the table's `:protocol_max_table_size` is updated accordingly. |
lib/hpax/table.ex
Outdated
| {new_entries_reversed, new_size} = evict_towards_size(Enum.reverse(entries), size, new_size) | ||
| def resize(%__MODULE__{max_table_size: max_table_size} = table, new_protocol_max_table_size) | ||
| when new_protocol_max_table_size >= max_table_size do | ||
| %{ |
There was a problem hiding this comment.
Until we have the type system... 😄
Suggested change
| %{ | |
| %__MODULE__{ |
There was a problem hiding this comment.
And in other places too
lib/hpax/table.ex
Outdated
| Additionally, if the current max table size is larger than this value, it is also included int | ||
| the list, per https://www.rfc-editor.org/rfc/rfc7541#section-4.2 | ||
| """ | ||
| def pending_resizes(%{pending_minimum_resize: nil} = table), do: {table, []} |
There was a problem hiding this comment.
Since "returns and clears", let's call this
Suggested change
| def pending_resizes(%{pending_minimum_resize: nil} = table), do: {table, []} | |
| def pop_pending_resizes(%{pending_minimum_resize: nil} = table), do: {table, []} |
?
|
Issues addressed! |
|
Fantastic work @mtrudel thank you! 💟 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #18.
protocol_max_table_sizefield on tables, which records the containing protocol (ie: HTTP/2's) view of the max allowable table size. Dynamic resizes validate the new size against this value.max_table_sizevalue on tables (evicting as needed to satisfy)max_protocol_table_sizeon the encoder are signalled to the decoder via dynamic table size update messages