Skip to content

Conversation

@TingDaoK
Copy link
Contributor

Issue #, if available:

Description of changes:

  • settings change from internal.
  • create a queue for pending settings
  • when we want to change our settings, we send a setting frame to our peer, then push the pending settings to the back of the queue.
  • when the setting ACK is received from the peer, we pop the front one from the queue, and apply the settings to our connection.

@TingDaoK TingDaoK requested a review from a team April 17, 2020 18:05
@TingDaoK TingDaoK changed the title internal settings internal settings change function Apr 17, 2020
source/hpack.c Outdated
if (!context->dynamic_table_size_update.pending) {
context->dynamic_table_size_update.pending = true;
void aws_hpack_set_max_table_size(struct aws_hpack_context *context, size_t setting_max_size, bool update_table_size) {
if (update_table_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot figure out the purpose of the update_table_size variable?

the dynamic_table_size_update struct is only used by the "encoder" part of aws_hpack_context (hence, my comment about it being confusing that encoder/decoder share a class)

/* The new maximum size MUST be lower than or equal to the limit determined by the protocol using HPACK.
* A value that exceeds this limit MUST be treated as a decoding error. */
if (*size64 > context->dynamic_table.protocol_max_size_setting) {
HPACK_LOG(ERROR, context, "Dynamic table update size is larger than the protocal setting");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure this check is correct
maybe we should just remove all the changes to hpack for now and tackle it in another PR

Copy link
Contributor

@graebm graebm left a comment

Choose a reason for hiding this comment

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

B E A U T I F U L

aws_h2_connection_enqueue_outgoing_frame(connection, setting_frame);

aws_linked_list_push_back(&connection->thread_data.pending_settings_queue, &pending_settings->node);
return AWS_OP_SUCCESS;
Copy link
Contributor

Choose a reason for hiding this comment

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

If aws_h2_connection_change_settings() is going to be public-ish, let's have it s_try_write_outgoing_frames() at the end

if (aws_h2_connection_change_settings(connection, initial_settings, new_setting_iter)) {
CONNECTION_LOGF(
ERROR,
connection,
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the s_try_write_outgoing_frames() below this if we move it to aws_h2_connection_change_settings()

@TingDaoK TingDaoK merged commit e684773 into master Apr 20, 2020
@TingDaoK TingDaoK deleted the internal-settings-based-on-master branch April 20, 2020 22:13
czakian pushed a commit to czakian/aws-c-http that referenced this pull request Apr 24, 2020
settings change from internal.
- create a queue for pending settings
- when we want to change our settings, we send a setting frame to our peer, then push the pending settings to the back of the queue.
- when the setting ACK is received from the peer, we pop the front one from the queue and apply the settings to our connection.
Co-authored-by: Dengke Tang <dengket@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants