encode/httpx#1214 prefer sending outbound cookies separately to improve headers compression by cdeler · Pull Request #1275 · python-hyper/h2

cdeler · 2022-12-28T05:28:23Z

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?

src/h2/utilities.py

Kriechi · 2022-12-28T11:06:27Z

src/h2/utilities.py

+                    if isinstance(header, HeaderTuple):
+                        yield header.__class__(header[0], cookie_val)
+                    else:
+                        yield header[0], cookie_val


At this point, what are the trade-offs to simply always create HeaderTuple's?

Since we use the same approach in

h2/src/h2/utilities.py

Lines 549 to 552 in c7f967d

if isinstance(header, HeaderTuple):

yield header.__class__(header[0].strip(), header[1].strip())

else:

yield (header[0].strip(), header[1].strip())

, I don't mind either way.

I'm not sure about that, but I see that all outbound cookies are handled using this pattern: isinstance(header, HeaderTuple): ...

h2/src/h2/utilities.py

Lines 534 to 538 in 63b6b97

for header in headers:

if isinstance(header, HeaderTuple):

yield header.__class__(header[0].lower(), header[1])

else:

yield (header[0].lower(), header[1])

h2/src/h2/utilities.py

Lines 548 to 552 in 63b6b97

for header in headers:

if isinstance(header, HeaderTuple):

yield header.__class__(header[0].strip(), header[1].strip())

else:

yield (header[0].strip(), header[1].strip())

Also I'm not sure about using NeverIndexedHeaderTuple (e.g. how it can negatively affect compression)

sethmlarson · 2022-12-28T15:41:13Z

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 · 2022-12-28T15:51:17Z

@sethmlarson I asked myself the same thing, and found this:

h2/src/h2/utilities.py

Lines 583 to 603 in c7f967d

    
           def _combine_cookie_fields(headers, hdr_validation_flags): 
        
               """ 
        
               RFC 7540 § 8.1.2.5 allows HTTP/2 clients to split the Cookie header field, 
        
               which must normally appear only once, into multiple fields for better 
        
               compression. However, they MUST be joined back up again when received. 
        
               This normalization step applies that transform. The side-effect is that 
        
               all cookie fields now appear *last* in the header block. 
        
               """ 
        
               # There is a problem here about header indexing. Specifically, it's 
        
               # possible that all these cookies are sent with different header indexing 
        
               # values. At this point it shouldn't matter too much, so we apply our own 
        
               # logic and make them never-indexed. 
        
               cookies = [] 
        
               for header in headers: 
        
                   if header[0] == b'cookie': 
        
                       cookies.append(header[1]) 
        
                   else: 
        
                       yield header 
        
               if cookies: 
        
                   cookie_val = b'; '.join(cookies) 
        
                   yield NeverIndexedHeaderTuple(b'cookie', cookie_val)

Kriechi · 2023-01-02T22:37:10Z

as a first step to a review, I fixed our project CI (which was a challenge by itself).
#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?

…to improve headers compression

cdeler · 2023-01-07T05:32:46Z

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 · 2023-01-07T13:02:28Z

LGTM!

kuntaoKZ · 2024-01-31T13:50:26Z

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

Kriechi reviewed Dec 28, 2022

View reviewed changes

cdeler requested a review from Kriechi January 2, 2023 05:22

cdeler force-pushed the cdeler-split-outbound-cookie-headers branch from 48f26a2 to 65b719c Compare January 3, 2023 06:52

encode/httpx#1214 add an ability to send outbound cookies separately …

a48a2fe

…to improve headers compression

cdeler force-pushed the cdeler-split-outbound-cookie-headers branch from 65b719c to a48a2fe Compare January 7, 2023 05:29

Kriechi approved these changes Jan 11, 2023

View reviewed changes

Kriechi merged commit 0c15f1f into python-hyper:master Jan 11, 2023

cdeler deleted the cdeler-split-outbound-cookie-headers branch January 11, 2023 21:42

Kriechi added a commit to Kriechi/hyper-h2 that referenced this pull request Dec 21, 2024

fix backward-compatibility of semi-public API for python-hyper#1275

a6cb12d

Kriechi added a commit to Kriechi/hyper-h2 that referenced this pull request Jan 22, 2025

fix backward-compatibility of semi-public API for python-hyper#1275

9151e38

Kriechi added a commit that referenced this pull request Jan 22, 2025

fix backward-compatibility of semi-public API for #1275

9bfb877

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

encode/httpx#1214 prefer sending outbound cookies separately to improve headers compression#1275
Kriechi merged 1 commit intopython-hyper:masterfrom
cdeler:cdeler-split-outbound-cookie-headers

cdeler commented Dec 28, 2022

Uh oh!

Uh oh!

Kriechi Dec 28, 2022

Uh oh!

Kriechi Dec 28, 2022

Uh oh!

cdeler Dec 28, 2022

Uh oh!

sethmlarson commented Dec 28, 2022

Uh oh!

Kriechi commented Dec 28, 2022

Uh oh!

Kriechi commented Jan 2, 2023

Uh oh!

cdeler commented Jan 7, 2023

Uh oh!

Kriechi commented Jan 7, 2023

Uh oh!

kuntaoKZ commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if isinstance(header, HeaderTuple):
	yield header.__class__(header[0].strip(), header[1].strip())
	else:
	yield (header[0].strip(), header[1].strip())

	for header in headers:
	if isinstance(header, HeaderTuple):
	yield header.__class__(header[0].lower(), header[1])
	else:
	yield (header[0].lower(), header[1])

Conversation

cdeler commented Dec 28, 2022

Notes:

Uh oh!

Uh oh!

Kriechi Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

Kriechi Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

cdeler Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

sethmlarson commented Dec 28, 2022

Uh oh!

Kriechi commented Dec 28, 2022

Uh oh!

Kriechi commented Jan 2, 2023

Uh oh!

cdeler commented Jan 7, 2023

Uh oh!

Kriechi commented Jan 7, 2023

Uh oh!

kuntaoKZ commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants