Merge pull request #51 from python-hyper/awlc/stream-0

Improve error messages when handling stream 0

HISTORY.rst

@@ -8,6 +8,10 @@ Changelog
 
 - Throw ``BadWeightError`` when creating or reprioritising a stream with a
   weight that is not an integer between 1 and 256, inclusive.
+- Throw ``PseudoStreamError`` when trying to reprioritise, remove, block or
+  unblock stream 0.
+- Add a new ``PriorityError`` parent class for the exceptions that can be
+  thrown by priority.
 
 1.2.2 (2016-11-11)
 ------------------

docs/source/api.rst

@@ -10,6 +10,8 @@ Priority Tree
 Exceptions
 ----------
 
+.. autoclass:: priority.PriorityError
+
 .. autoclass:: priority.DeadlockError
 
 .. autoclass:: priority.PriorityLoop
@@ -19,3 +21,7 @@ Exceptions
 .. autoclass:: priority.MissingStreamError
 
 .. autoclass:: priority.TooManyStreamsError
+
+.. autoclass:: priority.BadWeightError
+
+.. autoclass:: priority.PseudoStreamError

src/priority/__init__.py

@@ -4,5 +4,5 @@
 """
 from .priority import (  # noqa
     Stream, PriorityTree, DeadlockError, PriorityLoop, DuplicateStreamError,
-    MissingStreamError, TooManyStreamsError, BadWeightError
+    MissingStreamError, TooManyStreamsError, BadWeightError, PseudoStreamError
 )

src/priority/priority.py

@@ -14,36 +14,42 @@
 PY3 = sys.version_info[0] == 3
 
 
-class DeadlockError(Exception):
+class PriorityError(Exception):
+    """
+    The base class for all ``priority`` exceptions.
+    """
+
+
+class DeadlockError(PriorityError):
     """
     Raised when there are no streams that can make progress: all streams are
     blocked.
     """
     pass
 
 
-class PriorityLoop(Exception):
+class PriorityLoop(PriorityError):
     """
     An unexpected priority loop has been detected. The tree is invalid.
     """
     pass
 
 
-class DuplicateStreamError(Exception):
+class DuplicateStreamError(PriorityError):
     """
     An attempt was made to insert a stream that already exists.
     """
     pass
 
 
-class MissingStreamError(KeyError, Exception):
+class MissingStreamError(KeyError, PriorityError):
     """
     An operation was attempted on a stream that is not present in the tree.
     """
     pass
 
 
-class TooManyStreamsError(Exception):
+class TooManyStreamsError(PriorityError):
     """
     An attempt was made to insert a dangerous number of streams into the
     priority tree at the same time.
@@ -53,7 +59,7 @@ class TooManyStreamsError(Exception):
     pass
 
 
-class BadWeightError(Exception):
+class BadWeightError(PriorityError):
     """
     An attempt was made to create a stream with an invalid weight.
 
@@ -62,6 +68,15 @@ class BadWeightError(Exception):
     pass
 
 
+class PseudoStreamError(PriorityError):
+    """
+    An operation was attempted on stream 0.
+
+    .. versionadded:: 1.3.0
+    """
+    pass
+
+
 class Stream(object):
     """
     Priority information for a given stream.
@@ -228,6 +243,27 @@ def __ge__(self, other):
         return self.stream_id >= other.stream_id
 
 
+def _stream_cycle(new_parent, current):
+    """
+    Reports whether the new parent depends on the current stream.
+    """
+    parent = new_parent
+
+    # Don't iterate forever, but instead assume that the tree doesn't
+    # get more than 100 streams deep. This should catch accidental
+    # tree loops. This is the definition of defensive programming.
+    for _ in range(100):
+        parent = parent.parent
+        if parent.stream_id == current.stream_id:
+            return True
+        elif parent.stream_id == 0:
+            return False
+
+    raise PriorityLoop(
+        "Stream %d is in a priority loop." % new_parent.stream_id
+    )  # pragma: no cover
+
+
 class PriorityTree(object):
     """
     A HTTP/2 Priority Tree.
@@ -339,25 +375,8 @@ def reprioritize(self,
         :param exclusive: (optional) Whether this stream should now be an
             exclusive dependency of the new parent.
         """
-        def stream_cycle(new_parent, current):
-            """
-            Reports whether the new parent depends on the current stream.
-            """
-            parent = new_parent
-
-            # Don't iterate forever, but instead assume that the tree doesn't
-            # get more than 100 streams deep. This should catch accidental
-            # tree loops. This is the definition of defensive programming.
-            for _ in range(100):
-                parent = parent.parent
-                if parent.stream_id == current.stream_id:
-                    return True
-                elif parent.stream_id == 0:
-                    return False
-
-            raise PriorityLoop(
-                "Stream %d is in a priority loop." % new_parent.stream_id
-            )  # pragma: no cover
+        if stream_id == 0:
+            raise PseudoStreamError("Cannot reprioritize stream 0")
 
         try:
             current_stream = self._streams[stream_id]
@@ -371,7 +390,7 @@ def stream_cycle(new_parent, current):
         # and move it to its new parent, taking its children with it.
         if depends_on:
             new_parent = self._get_or_insert_parent(depends_on)
-            cycle = stream_cycle(new_parent, current_stream)
+            cycle = _stream_cycle(new_parent, current_stream)
         else:
             new_parent = self._streams[0]
             cycle = False
@@ -400,6 +419,9 @@ def remove_stream(self, stream_id):
 
         :param stream_id: The ID of the stream to remove.
         """
+        if stream_id == 0:
+            raise PseudoStreamError("Cannot remove stream 0")
+
         try:
             child = self._streams.pop(stream_id)
         except KeyError:
@@ -414,6 +436,9 @@ def block(self, stream_id):
 
         :param stream_id: The ID of the stream to block.
         """
+        if stream_id == 0:
+            raise PseudoStreamError("Cannot block stream 0")
+
         try:
             self._streams[stream_id].active = False
         except KeyError:
@@ -425,7 +450,9 @@ def unblock(self, stream_id):
 
         :param stream_id: The ID of the stream to unblock.
         """
-        # When a stream becomes unblocked,
+        if stream_id == 0:
+            raise PseudoStreamError("Cannot unblock stream 0")
+
         try:
             self._streams[stream_id].active = True
         except KeyError:

test/test_priority.py

@@ -296,6 +296,25 @@ def test_priority_raises_good_errors_for_missing_streams(self):
         with pytest.raises(priority.MissingStreamError):
             p.remove_stream(3)
 
+    def test_priority_raises_good_errors_for_zero_stream(self):
+        """
+        Attempting operations on stream 0 raises a PseudoStreamError.
+        """
+        p = priority.PriorityTree()
+        p.insert_stream(1)
+
+        with pytest.raises(priority.PseudoStreamError):
+            p.reprioritize(0)
+
+        with pytest.raises(priority.PseudoStreamError):
+            p.block(0)
+
+        with pytest.raises(priority.PseudoStreamError):
+            p.unblock(0)
+
+        with pytest.raises(priority.PseudoStreamError):
+            p.remove_stream(0)
+
     @pytest.mark.parametrize('exclusive', [True, False])
     def test_priority_allows_inserting_stream_with_absent_parent(self,
                                                                  exclusive):