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

Add a setter for header_table_size #638

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Add a setter for header_table_size #638

merged 2 commits into from
Oct 30, 2023

Conversation

4JX
Copy link
Contributor

@4JX 4JX commented Aug 23, 2022

The function's description may need some refinement

Closes #637

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Nice work! What do you think of adding a test to tests/h2-tests/src/client.rs?

@4JX
Copy link
Contributor Author

4JX commented Aug 24, 2022

Nice work! What do you think of adding a test to tests/h2-tests/src/client.rs?

Used the file that most closely matched what you said. Basically a carbon copy of client_builder_max_concurrent_streams.

@seanmonstar
Copy link
Member

Thanks for including the test! I think this is probably good to go... Though, I'd want to ask you, since you're thinking about this topic, you might have certain expected behavior in mind. Should we be testing for that behavior?

@4JX
Copy link
Contributor Author

4JX commented Aug 25, 2022

What I need is covered by the current test (and small fix). That is, the setting is included in the SETTINGS frame with the value set by the user (and the client successfully handles requests with that setting enabled).

Might be worth to also test that it will throw a protocol error in the server requested size > header_table_size case? (Though I'm not sure if this should be another function / part of the same test / part of the codec read ones)

@alexforster
Copy link

@seanmonstar Is there anything I can do to help this get merged?

@seanmonstar
Copy link
Member

I don't remember what test I was asking for... What's here seems good. Just needs conflicts resolved and we can merge.

@4JX
Copy link
Contributor Author

4JX commented Oct 23, 2023

Rebased onto master and squashed the commits together. Did compile and pass the tests locally.

@4JX
Copy link
Contributor Author

4JX commented Oct 29, 2023

Just making sure this is not waiting on CI? Doesn't seem like the error is related to code touched by this PR.

@seanmonstar
Copy link
Member

Sorry, I fixed CI in #722, restarting it here.

@seanmonstar seanmonstar merged commit ef743ec into hyperium:master Oct 30, 2023
7 checks passed
0xE282B0 pushed a commit to 0xE282B0/h2 that referenced this pull request Jan 11, 2024
0xE282B0 pushed a commit to 0xE282B0/h2 that referenced this pull request Jan 11, 2024
0xE282B0 pushed a commit to 0xE282B0/h2 that referenced this pull request Jan 16, 2024
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.

Add a setter for header_table_size
3 participants