encode/httpx#1214 prefer sending outbound cookies separately to improve headers compression

python-hyper / h2

HTTP/2 State-Machine based protocol implementation

https://h2.readthedocs.io/en/stable

MIT License

945 stars 152 forks source link

encode/httpx#1214 prefer sending outbound cookies separately to improve headers compression #1275

Closed cdeler closed 1 year ago

cdeler commented 1 year ago

According to the rfc7540#section-8.1.2.5,

To allow for better compression efficiency, the Cookie header field MAY be split into separate header fields

The h2 framework is used by the httpx client, and some users require to have an ability to send cookies via separate headers to follow http2 practice. Or at least to make this option configurable (more: encode/httpx#1214).

As h2 decides how to properly send data over the transport, h2 seems to be a right place to add an ability to split the cookie headers. Also the opposite functionality is added to the inbound cookies processing (ref).

Notes:

I have concerns about my PR, may be you can help me with suggestings:

I used the "server" connection in the client connections tests. I don't know if it's a right way to do
may be it makes sense to hide this feature under a config? If so, do you have suggestions where to add it?

sethmlarson commented 1 year ago

Question for some context: if a client sends Cookie headers that are already split does h2 combine them somewhere or will the split header fields be encoded as expected?

Kriechi commented 1 year ago

@sethmlarson I asked myself the same thing, and found this: https://github.com/python-hyper/h2/blob/c7f967d77a6f32be374e33adca195e65a00afb63/src/h2/utilities.py#L583-L603

Kriechi commented 1 year ago

as a first step to a review, I fixed our project CI (which was a challenge by itself). https://github.com/python-hyper/h2/pull/1276

I don't see any obvious issues with the code itself - though I still need time to think and read through the spec and implications. Happy to hear other thoughts in the mean time!

@cdeler you said:

some users require to have an ability and to add an ability to split

I don't see an "option" in your PR - its "always on"? Or do you think this should be the new one-and-only way to handle cookies?

cdeler commented 1 year ago

Hello @Kriechi

Or do you think this should be the new one-and-only way to handle cookies?

I don't think so (because RFC says "may") :-) , so that I updated the PR adding split_outbound_cookies flag to the h2.config.H2Configuration (False by default)

Kriechi commented 1 year ago

LGTM!

kuntaoKZ commented 5 months ago

Hello, seems this change was not covered in 4.1.0 version?