Fix handling of no data in hiredis-py

Use a sentinel object instance to signal lack of data in hiredis-py, instead of piggybacking of `False`, which can also be returned by parsing valid RESP payloads.
redis · Jun 12, 2024 · a9dabf9 · a9dabf9
1 parent 030869e
commit a9dabf9
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 29 deletions.
diff --git a/redis/_parsers/hiredis.py b/redis/_parsers/hiredis.py
@@ -19,6 +19,11 @@
     SERVER_CLOSED_CONNECTION_ERROR,
 )
 
+# Used to signal that hiredis-py does not have enough data to parse.
+# Using `False` or `None` is not reliable, given that the parser can
+# return `False` or `None` for legitimate reasons from RESP payloads.
+NOT_ENOUGH_DATA = object()
+
 
 class _HiredisReaderArgs(TypedDict, total=False):
     protocolError: Callable[[str], Exception]
@@ -51,25 +56,26 @@ def on_connect(self, connection, **kwargs):
             "protocolError": InvalidResponse,
             "replyError": self.parse_error,
             "errors": connection.encoder.encoding_errors,
+            "notEnoughData": NOT_ENOUGH_DATA,
         }
 
         if connection.encoder.decode_responses:
             kwargs["encoding"] = connection.encoder.encoding
         self._reader = hiredis.Reader(**kwargs)
-        self._next_response = False
+        self._next_response = NOT_ENOUGH_DATA
 
     def on_disconnect(self):
         self._sock = None
         self._reader = None
-        self._next_response = False
+        self._next_response = NOT_ENOUGH_DATA
 
     def can_read(self, timeout):
         if not self._reader:
             raise ConnectionError(SERVER_CLOSED_CONNECTION_ERROR)
 
-        if self._next_response is False:
+        if self._next_response is NOT_ENOUGH_DATA:
             self._next_response = self._reader.gets()
-            if self._next_response is False:
+            if self._next_response is NOT_ENOUGH_DATA:
                 return self.read_from_socket(timeout=timeout, raise_on_timeout=False)
         return True
 
@@ -108,17 +114,17 @@ def read_response(self, disable_decoding=False):
             raise ConnectionError(SERVER_CLOSED_CONNECTION_ERROR)
 
         # _next_response might be cached from a can_read() call
-        if self._next_response is not False:
+        if self._next_response is not NOT_ENOUGH_DATA:
             response = self._next_response
-            self._next_response = False
+            self._next_response = NOT_ENOUGH_DATA
             return response
 
         if disable_decoding:
             response = self._reader.gets(False)
         else:
             response = self._reader.gets()
 
-        while response is False:
+        while response is NOT_ENOUGH_DATA:
             self.read_from_socket()
             if disable_decoding:
                 response = self._reader.gets(False)
@@ -156,6 +162,7 @@ def on_connect(self, connection):
         kwargs: _HiredisReaderArgs = {
             "protocolError": InvalidResponse,
             "replyError": self.parse_error,
+            "notEnoughData": NOT_ENOUGH_DATA,
         }
         if connection.encoder.decode_responses:
             kwargs["encoding"] = connection.encoder.encoding
@@ -170,7 +177,7 @@ def on_disconnect(self):
     async def can_read_destructive(self):
         if not self._connected:
             raise ConnectionError(SERVER_CLOSED_CONNECTION_ERROR)
-        if self._reader.gets():
+        if self._reader.gets() is not NOT_ENOUGH_DATA:
             return True
         try:
             async with async_timeout(0):
@@ -200,7 +207,7 @@ async def read_response(
             response = self._reader.gets(False)
         else:
             response = self._reader.gets()
-        while response is False:
+        while response is NOT_ENOUGH_DATA:
             await self.read_from_socket()
             if disable_decoding:
                 response = self._reader.gets(False)

diff --git a/tests/test_asyncio/test_bloom.py b/tests/test_asyncio/test_bloom.py
@@ -42,8 +42,6 @@ async def test_tdigest_create(decoded_r: redis.Redis):
     assert await decoded_r.tdigest().create("tDigest", 100)
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 async def test_bf_add(decoded_r: redis.Redis):
     assert await decoded_r.bf().create("bloom", 0.01, 1000)
@@ -57,8 +55,6 @@ async def test_bf_add(decoded_r: redis.Redis):
     assert [1, 0] == intlist(await decoded_r.bf().mexists("bloom", "foo", "noexist"))
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 async def test_bf_insert(decoded_r: redis.Redis):
     assert await decoded_r.bf().create("bloom", 0.01, 1000)
@@ -90,8 +86,6 @@ async def test_bf_insert(decoded_r: redis.Redis):
     )
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 async def test_bf_scandump_and_loadchunk(decoded_r: redis.Redis):
     # Store a filter
@@ -191,8 +185,6 @@ async def test_bf_card(decoded_r: redis.Redis):
         await decoded_r.bf().card("setKey")
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 async def test_cf_add_and_insert(decoded_r: redis.Redis):
     assert await decoded_r.cf().create("cuckoo", 1000)
@@ -219,8 +211,6 @@ async def test_cf_add_and_insert(decoded_r: redis.Redis):
     )
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 async def test_cf_exists_and_del(decoded_r: redis.Redis):
     assert await decoded_r.cf().create("cuckoo", 1000)

diff --git a/tests/test_bloom.py b/tests/test_bloom.py
@@ -73,8 +73,6 @@ def test_tdigest_create(client):
     assert client.tdigest().create("tDigest", 100)
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 def test_bf_add(client):
     assert client.bf().create("bloom", 0.01, 1000)
@@ -88,8 +86,6 @@ def test_bf_add(client):
     assert [1, 0] == intlist(client.bf().mexists("bloom", "foo", "noexist"))
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 def test_bf_insert(client):
     assert client.bf().create("bloom", 0.01, 1000)
@@ -121,8 +117,6 @@ def test_bf_insert(client):
     )
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 def test_bf_scandump_and_loadchunk(client):
     # Store a filter
@@ -222,8 +216,6 @@ def test_bf_card(client):
         client.bf().card("setKey")
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 def test_cf_add_and_insert(client):
     assert client.cf().create("cuckoo", 1000)
@@ -250,8 +242,6 @@ def test_cf_add_and_insert(client):
     )
 
 
-# hiredis-py can't process well boolean responses
-@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
 @pytest.mark.redismod
 def test_cf_exists_and_del(client):
     assert client.cf().create("cuckoo", 1000)