search

hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 269 forks source link

Add a setter for `header_table_size` #638

Closed 4JX closed 8 months ago

4JX commented 1 year ago

The function's description may need some refinement

Closes #637

4JX commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 8 months ago

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

seanmonstar commented 8 months ago

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

4JX commented 8 months ago

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

4JX commented 8 months ago

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

seanmonstar commented 8 months ago

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