From c28392d83c2c267cae42b9b852ef0d4ce029d2f6 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Tue, 24 May 2016 16:06:28 +0100 Subject: [PATCH 1/9] Basic tests for sending priority data. --- test/test_priority.py | 113 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/test/test_priority.py b/test/test_priority.py index 34fc35532..c34380ac9 100644 --- a/test/test_priority.py +++ b/test/test_priority.py @@ -113,3 +113,116 @@ def test_streams_may_not_depend_on_themselves(self, frame_factory): error_code=h2.errors.PROTOCOL_ERROR, ) assert c.data_to_send() == expected_frame.serialize() + + @pytest.mark.parametrize( + 'depends_on,weight,exclusive', + [ + (0, 256, False), + (3, 128, False), + (3, 128, True), + ] + ) + def test_can_prioritize_stream(self, depends_on, weight, exclusive, + frame_factory): + """ + hyper-h2 can emit priority frames. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + + c.send_headers(headers=self.example_request_headers, stream_id=1) + c.send_headers(headers=self.example_request_headers, stream_id=3) + c.clear_outbound_data_buffer() + + c.prioritize( + stream_id=1, + depends_on=depends_on, + weight=weight, + exclusive=exclusive + ) + + f = frame_factory.build_priority_frame( + stream_id=1, + weight=weight - 1, + depends_on=depends_on, + exclusive=exclusive, + ) + assert c.data_to_send() == f.serialize() + + @pytest.mark.parametrize( + 'depends_on,weight,exclusive', + [ + (0, 256, False), + (1, 128, False), + (1, 128, True), + ] + ) + def test_emit_headers_with_priority_info(self, depends_on, weight, + exclusive, frame_factory): + """ + It is possible to send a headers frame with priority information on + it. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.clear_outbound_data_buffer() + + c.send_headers( + headers=self.example_request_headers, + stream_id=3, + priority_weight=weight, + priority_depends_on=depends_on, + priority_exclusive=exclusive, + ) + + f = frame_factory.build_headers_frame( + headers=self.example_request_headers, + stream_id=3, + flags=['PRIORITY'], + stream_weight=weight - 1, + depends_on=depends_on, + exclusive=exclusive, + ) + assert c.data_to_send() == f.serialize() + + def test_may_not_prioritize_stream_to_depend_on_self(self, frame_factory): + """ + A stream adjusted to depend on itself causes a Protocol Error. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.receive_data(frame_factory.preamble()) + c.send_headers( + headers=self.example_request_headers, + stream_id=3, + priority_weight=255, + priority_depends_on=0, + priority_exclusive=False, + ) + c.clear_outbound_data_buffer() + + with pytest.raises(h2.exceptions.ProtocolError): + c.prioritize( + stream_id=3, + depends_on=3, + ) + + assert not c.data_to_send() + + def test_may_not_initially_set_stream_depend_on_self(self, frame_factory): + """ + A stream that starts by depending on itself causes a Protocol Error. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.receive_data(frame_factory.preamble()) + c.clear_outbound_data_buffer() + + with pytest.raises(h2.exceptions.ProtocolError): + c.send_headers( + headers=self.example_request_headers, + stream_id=3, + priority_depends_on=3, + ) + + assert not c.data_to_send() From d17785304334b851b4d78a92a50260f9ee920ea4 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Wed, 25 May 2016 13:56:29 +0100 Subject: [PATCH 2/9] Add prioritize method. --- h2/connection.py | 90 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/h2/connection.py b/h2/connection.py index 61e1225f2..6f4e6ea3d 100644 --- a/h2/connection.py +++ b/h2/connection.py @@ -982,6 +982,96 @@ def advertise_alternative_service(self, self._prepare_for_sending(frames) + def prioritize(self, stream_id, weight=16, depends_on=0, exclusive=False): + """ + Notify a server about the priority of a stream. + + Stream priorities are a form of guidance to a remote server: they + inform the server about how important a given response is, so that the + server may allocate its resources (e.g. bandwidth, CPU time, etc.) + accordingly. This exists to allow clients to ensure that the most + important data arrives earlier, while less important data does not + starve out the more important data. + + Stream priorities are explained in depth in `RFC 7540 Section 5.3 + `_. + + This method updates the priority information of a single stream. It may + be called well before a stream is actively in use, or well after a + stream is closed. + + .. warning:: RFC 7540 allows for servers to change the priority of + streams. However, hyper-h2 **does not** allow server + stacks to do this. This is because most clients do not + adequately know how to respond when provided conflicting + priority information, and relatively little utility is + provided by making that functionality available. + + .. note:: hyper-h2 **does not** maintain any information about the + RFC 7540 priority tree. That means that hyper-h2 does not + prevent incautious users from creating invalid priority + trees, particularly by creating priority loops. While some + basic error checking is provided by hyper-h2, users are + strongly recommended to understand their prioritisation + strategies before using the priority tools here. + + .. note:: Priority information is strictly advisory. Servers are + allowed to disregard it entirely. Avoid relying on the idea + that your priority signaling will definitely be obeyed. + + .. versionadded:: 2.4.0 + + :param stream_id: The ID of the stream to prioritize. + :type stream_id: ``int`` + + :param weight: The weight to give the stream. Defaults to ``16``, the + default weight of any stream. May be any value between ``1`` and + ``256`` inclusive. The relative weight of a stream indicates what + proportion of available resources will be allocated to that + stream. + :type weight: ``int`` + + :param depends_on: The ID of the stream on which this stream depends. + This stream will only be progressed if it is impossible to + progress the parent stream (the one on which this one depends). + Passing the value ``0`` means that this stream does not depend on + any other. Defaults to ``0``. + :type depends_on: ``int`` + + :param exclusive: Whether this stream is an exclusive dependency of its + "parent" stream (i.e. the stream given by ``depends_on``). If a + stream is an exclusive dependency of another, that means that all + previously-set children of the parent are moved to become children + of the new exclusively-dependent stream. Defaults to ``False``. + :type exclusive: ``bool`` + """ + self.state_machine.process_input( + ConnectionInputs.SEND_PRIORITY + ) + + # A stream may not depend on itself. + if depends_on == stream_id: + raise ProtocolError( + "Stream %d may not depend on itself" % stream_id + ) + + # Weight must be between 1 and 256. + if weight > 256 or weight < 1: + raise ProtocolError( + "Weight must be between 1 and 256, not %d" % weight + ) + + frame = PriorityFrame() + frame.stream_id = stream_id + frame.exclusive = exclusive + frame.depends_on = depends_on + + # Weight is an integer between 1 and 256, but the byte only allows 0 + # to 255: subtract one. + frame.weight = weight - 1 + + self._prepare_for_sending([frame]) + def local_flow_control_window(self, stream_id): """ Returns the maximum amount of data that can be sent on stream From aabd374b44c69b9847d566ae3495e2705ef2be46 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 27 May 2016 13:45:31 +0100 Subject: [PATCH 3/9] More priority tests. --- test/test_priority.py | 84 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/test/test_priority.py b/test/test_priority.py index c34380ac9..ffedfa7f9 100644 --- a/test/test_priority.py +++ b/test/test_priority.py @@ -226,3 +226,87 @@ def test_may_not_initially_set_stream_depend_on_self(self, frame_factory): ) assert not c.data_to_send() + + @pytest.mark.parametrize('weight', [0, -15, 257]) + def test_prioritize_requires_valid_weight(self, weight): + """ + A call to prioritize with an invalid weight causes a ProtocolError. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.clear_outbound_data_buffer() + + with pytest.raises(h2.exceptions.ProtocolError): + c.prioritize(stream_id=1, weight=weight) + + assert not c.data_to_send() + + @pytest.mark.parametrize('weight', [0, -15, 257]) + def test_send_headers_requires_valid_weight(self, weight): + """ + A call to send_headers with an invalid weight causes a ProtocolError. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.clear_outbound_data_buffer() + + with pytest.raises(h2.exceptions.ProtocolError): + c.send_headers( + stream_id=1, + headers=self.example_request_headers, + priority_weight=weight + ) + + assert not c.data_to_send() + + def test_prioritize_defaults(self, frame_factory): + """ + When prioritize() is called with no explicit arguments, it emits a + weight of 16, depending on stream zero non-exclusively. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.clear_outbound_data_buffer() + + c.prioritize(stream_id=1) + + f = frame_factory.build_priority_frame( + stream_id=1, + weight=15, + depends_on=0, + exclusive=False, + ) + assert c.data_to_send() == f.serialize() + + @pytest.mark.parametrize( + 'priority_kwargs', + [ + {'priority_weight': 16}, + {'priority_depends_on': 0}, + {'priority_exclusive': False}, + ] + ) + def test_send_headers_defaults(self, priority_kwargs, frame_factory): + """ + When send_headers() is called with only one explicit argument, it emits + default values for everything else. + """ + c = h2.connection.H2Connection() + c.initiate_connection() + c.clear_outbound_data_buffer() + + c.send_headers( + stream_id=1, + headers=self.example_request_headers, + **priority_kwargs + ) + + f = frame_factory.build_headers_frame( + headers=self.example_request_headers, + stream_id=1, + flags=['PRIORITY'], + stream_weight=15, + depends_on=0, + exclusive=False, + ) + assert c.data_to_send() == f.serialize() From bfbed52ec2ec3345309b95347468db420a8db01b Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 27 May 2016 13:45:53 +0100 Subject: [PATCH 4/9] Add priority implementation. --- h2/connection.py | 124 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 102 insertions(+), 22 deletions(-) diff --git a/h2/connection.py b/h2/connection.py index 6f4e6ea3d..331a2e18b 100644 --- a/h2/connection.py +++ b/h2/connection.py @@ -589,7 +589,9 @@ def get_next_available_stream_id(self): return next_stream_id - def send_headers(self, stream_id, headers, end_stream=False): + def send_headers(self, stream_id, headers, end_stream=False, + priority_weight=None, priority_depends_on=None, + priority_exclusive=None): """ Send headers on a given stream. @@ -631,6 +633,13 @@ def send_headers(self, stream_id, headers, end_stream=False): ` and :class:`NeverIndexedHeaderTuple ` objects. + This method also allows users to prioritize the stream immediately, + by sending priority information on the HEADERS frame directly. To do + this, any one of ``priority_weight``, ``priority_depends_on``, or + ``priority_exclusive`` must be set to a value that is not ``None``. For + more information on the priority fields, see :meth:`prioritize + `. + .. warning:: In HTTP/2, it is mandatory that all the HTTP/2 special headers (that is, ones whose header keys begin with ``:``) appear at the start of the header block, before any normal headers. @@ -643,12 +652,43 @@ def send_headers(self, stream_id, headers, end_stream=False): Added support for using :class:`HeaderTuple ` objects to store headers. + .. versionchanged:: 2.4.0 + Added the ability to provide priority keyword arguments: + ``priority_weight``, ``priority_depends_on``, and + ``priority_exclusive``. + :param stream_id: The stream ID to send the headers on. If this stream does not currently exist, it will be created. :type stream_id: ``int`` + :param headers: The request/response headers to send. :type headers: An iterable of two tuples of bytestrings or :class:`HeaderTuple ` objects. + + :param end_stream: Whether this headers frame should end the stream + immediately (that is, whether no more data will be sent after this + frame). Defaults to ``False``. + :type end_stream: ``bool`` + + :param priority_weight: Sets the priority weight of the stream. See + :meth:`prioritize ` for more about how + this field works. Defaults to ``None``, which means that no + priority information will be sent. + :type priority_weight: ``int`` or ``None`` + + :param priority_depends_on: Sets which stream this one depends on for + priority purposes. See :meth:`prioritize ` + for more about how this field works. Defaults to ``None``, which + means that no priority information will be sent. + :type priority_depends_on: ``int`` or ``None`` + + :param priority_exclusive: Sets whether this stream exclusively depends + on the stream given in ``priority_depends_on`` for priority + purposes. See :meth:`prioritize ` for more + about how this field workds. Defaults to ``None``, which means that + no priority information will be sent. + :type priority_depends_on: ``bool`` or ``None`` + :returns: Nothing """ # Check we can open the stream. @@ -667,6 +707,24 @@ def send_headers(self, stream_id, headers, end_stream=False): frames = stream.send_headers( headers, self.encoder, end_stream ) + + # We may need to send priority information. + priority_present = ( + (priority_weight is not None) or + (priority_depends_on is not None) or + (priority_exclusive is not None) + ) + + if priority_present: + headers_frame = frames[0] + headers_frame.flags.add('PRIORITY') + frames[0] = _add_frame_priority( + headers_frame, + priority_weight, + priority_depends_on, + priority_exclusive + ) + self._prepare_for_sending(frames) def send_data(self, stream_id, data, end_stream=False): @@ -982,7 +1040,8 @@ def advertise_alternative_service(self, self._prepare_for_sending(frames) - def prioritize(self, stream_id, weight=16, depends_on=0, exclusive=False): + def prioritize(self, stream_id, weight=None, depends_on=None, + exclusive=None): """ Notify a server about the priority of a stream. @@ -1049,26 +1108,8 @@ def prioritize(self, stream_id, weight=16, depends_on=0, exclusive=False): ConnectionInputs.SEND_PRIORITY ) - # A stream may not depend on itself. - if depends_on == stream_id: - raise ProtocolError( - "Stream %d may not depend on itself" % stream_id - ) - - # Weight must be between 1 and 256. - if weight > 256 or weight < 1: - raise ProtocolError( - "Weight must be between 1 and 256, not %d" % weight - ) - - frame = PriorityFrame() - frame.stream_id = stream_id - frame.exclusive = exclusive - frame.depends_on = depends_on - - # Weight is an integer between 1 and 256, but the byte only allows 0 - # to 255: subtract one. - frame.weight = weight - 1 + frame = PriorityFrame(stream_id) + frame = _add_frame_priority(frame, weight, depends_on, exclusive) self._prepare_for_sending([frame]) @@ -1672,3 +1713,42 @@ def _stream_id_is_outbound(self, stream_id): (one initiated by this peer), returns ``False`` otherwise. """ return (stream_id % 2 == int(self.client_side)) + + +def _add_frame_priority(frame, weight=None, depends_on=None, exclusive=None): + """ + Adds priority data to a given frame. Does not change any flags set on that + frame: if the caller is adding priority information to a HEADERS frame they + must set that themselves. + + This method also deliberately sets defaults for anything missing. + + This method validates the input values. + """ + # A stream may not depend on itself. + if depends_on == frame.stream_id: + raise ProtocolError( + "Stream %d may not depend on itself" % frame.stream_id + ) + + # Weight must be between 1 and 256. + if weight is not None: + if weight > 256 or weight < 1: + raise ProtocolError( + "Weight must be between 1 and 256, not %d" % weight + ) + else: + # Weight is an integer between 1 and 256, but the byte only allows + # 0 to 255: subtract one. + weight -= 1 + + # Set defaults for anything not provided. + weight = weight if weight is not None else 15 + depends_on = depends_on if depends_on is not None else 0 + exclusive = exclusive if exclusive is not None else False + + frame.stream_weight = weight + frame.depends_on = depends_on + frame.exclusive = exclusive + + return frame From adb82de9bb614891f9283e5491baf0f018c3b051 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Sun, 29 May 2016 14:08:10 -0700 Subject: [PATCH 5/9] Changelog for #233. --- HISTORY.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/HISTORY.rst b/HISTORY.rst index 369325fb5..36a144624 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -11,6 +11,10 @@ API Changes (Backward-Compatible) user to send additional debug data on the GOAWAY frame. - Adds ``last_stream_id`` to ``H2Connection.close_connection``, allowing the user to manually control what the reported last stream ID is. +- Add new method: ``prioritize``. +- Add support for emitting stream priority information when sending headers + frames using three new keyword arguments: ``priority_weight``, + ``priority_depends_on``, and ``priority_exclusive``. Bugfixes ~~~~~~~~ From 36d06a9f72656e2204aba61b2906dc262e869f4a Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Sun, 29 May 2016 14:31:57 -0700 Subject: [PATCH 6/9] Define RFC1122 error. --- h2/exceptions.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/h2/exceptions.py b/h2/exceptions.py index f06459882..5fa909bde 100644 --- a/h2/exceptions.py +++ b/h2/exceptions.py @@ -153,3 +153,18 @@ class UnsupportedFrameError(ProtocolError, KeyError): """ # TODO: Remove the KeyError in 3.0.0 pass + + +class RFC1122Error(H2Error): + """ + Emitted when users attempt to do something that is literally allowed by the + relevant RFC, but is sufficiently ill-defined that it's unwise to allow + users to actually do it. + + While there is some disagreement about whether or not we should be liberal + in what accept, it is a truth universally acknowledged that we should be + conservative in what emit. + """ + # shazow says I'm going to regret naming the exception this way. If that + # turns out to be true, TELL HIM NOTHING. + pass From 8d0df1101bde165d2617669652bbe43c79302c0a Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Sun, 29 May 2016 15:06:38 -0700 Subject: [PATCH 7/9] Test that servers cannot prioritize. --- test/test_priority.py | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/test_priority.py b/test/test_priority.py index ffedfa7f9..7830a419c 100644 --- a/test/test_priority.py +++ b/test/test_priority.py @@ -24,6 +24,10 @@ class TestPriority(object): (':scheme', 'https'), (':method', 'GET'), ] + example_response_headers = [ + (':status', '200'), + ('server', 'pytest-h2'), + ] def test_receiving_priority_emits_priority_update(self, frame_factory): """ @@ -310,3 +314,43 @@ def test_send_headers_defaults(self, priority_kwargs, frame_factory): exclusive=False, ) assert c.data_to_send() == f.serialize() + + def test_servers_cannot_prioritize(self, frame_factory): + """ + Server stacks are not allowed to call ``prioritize()``. + """ + c = h2.connection.H2Connection(client_side=False) + c.initiate_connection() + c.receive_data(frame_factory.preamble()) + c.clear_outbound_data_buffer() + + f = frame_factory.build_headers_frame( + stream_id=1, + headers=self.example_request_headers, + ) + c.receive_data(f.serialize()) + + with pytest.raises(h2.exceptions.RFC1122Error): + c.prioritize(stream_id=1) + + def test_servers_cannot_prioritize_with_headers(self, frame_factory): + """ + Server stacks are not allowed to prioritize on headers either. + """ + c = h2.connection.H2Connection(client_side=False) + c.initiate_connection() + c.receive_data(frame_factory.preamble()) + c.clear_outbound_data_buffer() + + f = frame_factory.build_headers_frame( + stream_id=1, + headers=self.example_request_headers, + ) + c.receive_data(f.serialize()) + + with pytest.raises(h2.exceptions.RFC1122Error): + c.send_headers( + stream_id=1, + headers=self.example_response_headers, + priority_weight=16, + ) From d13f54007bb7f5b7d9b796aac567d43375d50847 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Sun, 29 May 2016 15:08:40 -0700 Subject: [PATCH 8/9] Document RFC1122Error --- docs/source/api.rst | 4 ++++ h2/exceptions.py | 2 ++ 2 files changed, 6 insertions(+) diff --git a/docs/source/api.rst b/docs/source/api.rst index 409eacae2..d4ad2f2a4 100644 --- a/docs/source/api.rst +++ b/docs/source/api.rst @@ -85,6 +85,10 @@ Exceptions :show-inheritance: :members: +.. autoclass:: h2.exceptions.RFC1122Error + :show-inheritance: + :members: + Protocol Errors ~~~~~~~~~~~~~~~ diff --git a/h2/exceptions.py b/h2/exceptions.py index 5fa909bde..242364761 100644 --- a/h2/exceptions.py +++ b/h2/exceptions.py @@ -164,6 +164,8 @@ class RFC1122Error(H2Error): While there is some disagreement about whether or not we should be liberal in what accept, it is a truth universally acknowledged that we should be conservative in what emit. + + .. versionadded:: 2.4.0 """ # shazow says I'm going to regret naming the exception this way. If that # turns out to be true, TELL HIM NOTHING. From 497232d0c6d5fae22afa092c588c2d9b5baeee24 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Sun, 29 May 2016 15:13:36 -0700 Subject: [PATCH 9/9] Prevent servers prioritising streams. --- h2/connection.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/h2/connection.py b/h2/connection.py index 331a2e18b..b2f59e151 100644 --- a/h2/connection.py +++ b/h2/connection.py @@ -27,7 +27,7 @@ from .exceptions import ( ProtocolError, NoSuchStreamError, FlowControlError, FrameTooLargeError, TooManyStreamsError, StreamClosedError, StreamIDTooLowError, - NoAvailableStreamIDError, UnsupportedFrameError + NoAvailableStreamIDError, UnsupportedFrameError, RFC1122Error ) from .frame_buffer import FrameBuffer from .settings import ( @@ -716,6 +716,9 @@ def send_headers(self, stream_id, headers, end_stream=False, ) if priority_present: + if not self.client_side: + raise RFC1122Error("Servers SHOULD NOT prioritize streams.") + headers_frame = frames[0] headers_frame.flags.add('PRIORITY') frames[0] = _add_frame_priority( @@ -1104,6 +1107,9 @@ def prioritize(self, stream_id, weight=None, depends_on=None, of the new exclusively-dependent stream. Defaults to ``False``. :type exclusive: ``bool`` """ + if not self.client_side: + raise RFC1122Error("Servers SHOULD NOT prioritize streams.") + self.state_machine.process_input( ConnectionInputs.SEND_PRIORITY )