Skip to content

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/h2/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@ class H2Configuration:
RFC 7540. Defaults to ``True``.
:type normalize_outbound_headers: ``bool``

:param split_outbound_cookies: Controls whether the outbound cookie
headers are split before sending or not. According to RFC 7540
- 8.1.2.5 the outbound header cookie headers may be split to improve
headers compression. Default is ``False``.
:type split_outbound_cookies: ``bool``

:param validate_inbound_headers: Controls whether the headers received
by this object are validated against the rules in RFC 7540.
Disabling this setting will cause inbound header validation to
Expand Down Expand Up @@ -148,6 +154,9 @@ class H2Configuration:
normalize_outbound_headers = _BooleanConfigOption(
'normalize_outbound_headers'
)
split_outbound_cookies = _BooleanConfigOption(
'split_outbound_cookies'
)
validate_inbound_headers = _BooleanConfigOption(
'validate_inbound_headers'
)
Expand All @@ -160,13 +169,15 @@ def __init__(self,
header_encoding=None,
validate_outbound_headers=True,
normalize_outbound_headers=True,
split_outbound_cookies=False,
validate_inbound_headers=True,
normalize_inbound_headers=True,
logger=None):
self.client_side = client_side
self.header_encoding = header_encoding
self.validate_outbound_headers = validate_outbound_headers
self.normalize_outbound_headers = normalize_outbound_headers
self.split_outbound_cookies = split_outbound_cookies
self.validate_inbound_headers = validate_inbound_headers
self.normalize_inbound_headers = normalize_inbound_headers
self.logger = logger or DummyLogger(__name__)
Expand Down
6 changes: 5 additions & 1 deletion src/h2/stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -1243,8 +1243,12 @@ def _build_headers_frames(self,
# We need to lowercase the header names, and to ensure that secure
# header fields are kept out of compression contexts.
if self.config.normalize_outbound_headers:
# also we may want to split outbound cookies to improve
# headers compression
should_split_outbound_cookies = self.config.split_outbound_cookies

headers = normalize_outbound_headers(
headers, hdr_validation_flags
headers, hdr_validation_flags, should_split_outbound_cookies
)
if self.config.validate_outbound_headers:
headers = validate_outbound_headers(
Expand Down
29 changes: 28 additions & 1 deletion src/h2/utilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -603,14 +603,41 @@ def _combine_cookie_fields(headers, hdr_validation_flags):
yield NeverIndexedHeaderTuple(b'cookie', cookie_val)


def normalize_outbound_headers(headers, hdr_validation_flags):
def _split_outbound_cookie_fields(headers, hdr_validation_flags):
"""
RFC 7540 § 8.1.2.5 allows for better compression efficiency,
to split the Cookie header field into separate header fields

We want to do it for outbound requests, as we are doing for
inbound.
"""
for header in headers:
if header[0] in (b'cookie', 'cookie'):
needle = b'; ' if isinstance(header[0], bytes) else '; '

if needle in header[1]:
for cookie_val in header[1].split(needle):
if isinstance(header, HeaderTuple):
yield header.__class__(header[0], cookie_val)
else:
yield header[0], cookie_val
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

else:
yield header
else:
yield header


def normalize_outbound_headers(headers, hdr_validation_flags, should_split_outbound_cookies):
"""
Normalizes a header sequence that we are about to send.

:param headers: The HTTP header set.
:param hdr_validation_flags: An instance of HeaderValidationFlags.
:param should_split_outbound_cookies: boolean flag
"""
headers = _lowercase_header_names(headers, hdr_validation_flags)
if should_split_outbound_cookies:
headers = _split_outbound_cookie_fields(headers, hdr_validation_flags)
headers = _strip_surrounding_whitespace(headers, hdr_validation_flags)
headers = _strip_connection_headers(headers, hdr_validation_flags)
headers = _secure_headers(headers, hdr_validation_flags)
Expand Down
43 changes: 43 additions & 0 deletions test/test_basic_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import hyperframe
import pytest
from hpack import HeaderTuple

import h2.config
import h2.connection
Expand Down Expand Up @@ -789,6 +790,48 @@ def test_headers_are_lowercase(self, frame_factory):

assert c.data_to_send() == expected_frame.serialize()

def test_outbound_cookie_headers_are_split(self):
"""
We should split outbound cookie headers according to
RFC 7540 - 8.1.2.5
"""
cookie_headers = [
HeaderTuple('cookie',
'username=John Doe; expires=Thu, 18 Dec 2013 12:00:00 UTC'),
('cookie', 'path=1'),
('cookie', 'test1=val1; test2=val2')
]

expected_cookie_headers = [
HeaderTuple('cookie', 'username=John Doe'),
HeaderTuple('cookie', 'expires=Thu, 18 Dec 2013 12:00:00 UTC'),
('cookie', 'path=1'),
('cookie', 'test1=val1'),
('cookie', 'test2=val2'),
]

client_config = h2.config.H2Configuration(
client_side=True,
header_encoding='utf-8',
split_outbound_cookies=True
)
server_config = h2.config.H2Configuration(
client_side=False,
normalize_inbound_headers=False,
header_encoding='utf-8'
)
client = h2.connection.H2Connection(config=client_config)
server = h2.connection.H2Connection(config=server_config)

client.initiate_connection()
client.send_headers(1, self.example_request_headers + cookie_headers, end_stream=True)

e = server.receive_data(client.data_to_send())

cookie_fields = [(n, v) for n, v in e[1].headers if n == 'cookie']

assert cookie_fields == expected_cookie_headers

@given(frame_size=integers(min_value=2**14, max_value=(2**24 - 1)))
@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
def test_changing_max_frame_size(self, frame_factory, frame_size):
Expand Down
4 changes: 2 additions & 2 deletions test/test_invalid_headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ def test_headers_event_skipping_validation(self, frame_factory, headers):
c.send_headers(1, headers)

# Ensure headers are still normalized.
norm_headers = h2.utilities.normalize_outbound_headers(headers, None)
norm_headers = h2.utilities.normalize_outbound_headers(headers, None, False)
f = frame_factory.build_headers_frame(norm_headers)
assert c.data_to_send() == f.serialize()

Expand All @@ -322,7 +322,7 @@ def test_push_promise_skipping_validation(self, frame_factory, headers):

# Create push promise frame with normalized headers.
frame_factory.refresh_encoder()
norm_headers = h2.utilities.normalize_outbound_headers(headers, None)
norm_headers = h2.utilities.normalize_outbound_headers(headers, None, False)
pp_frame = frame_factory.build_push_promise_frame(
stream_id=1, promised_stream_id=2, headers=norm_headers
)
Expand Down