Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

Fix buffer is closed error when using PythonParser class #1213

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/1212.bugfix
@@ -0,0 +1 @@
Fix buffer is closed error when using PythonParser class
1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
Expand Up @@ -38,6 +38,7 @@ Jeff Moser
Joongi Kim <achimnol>
Kyrylo Dehtyarenko
Leonid Shvechikov
Maksim Novikov
Manuel Miranda
Marek Szapiel
Marijn Giesen
Expand Down
2 changes: 1 addition & 1 deletion aioredis/connection.py
Expand Up @@ -373,7 +373,7 @@ def __init__(self, socket_read_size: int):
def on_connect(self, connection: "Connection"):
"""Called when the stream connects"""
self._stream = connection._reader
if self._buffer is None or self._stream is None:
if self._stream is None:
Copy link
Contributor Author

@m-novikov m-novikov Nov 20, 2021

Choose a reason for hiding this comment

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

I am not too sure what was the original intent behind this line, buffer only initialized after so it only can be None here

Copy link
Collaborator

Choose a reason for hiding this comment

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

i believe we've had unexpected behavior here before, so keeping that there would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self._stream is None check is kept and test seems to be passing with it, The problem was self._buffer is None which always resulted in an exception. As it's not possible for it to be something other than None at this point.

raise RedisError("Buffer is closed.")

self._buffer = SocketBuffer(
Expand Down
37 changes: 33 additions & 4 deletions tests/conftest.py
Expand Up @@ -8,7 +8,12 @@

import aioredis
from aioredis.client import Monitor
from aioredis.connection import parse_url
from aioredis.connection import (
HIREDIS_AVAILABLE,
HiredisParser,
PythonParser,
parse_url,
)

from .compat import mock

Expand Down Expand Up @@ -125,16 +130,40 @@ def skip_unless_arch_bits(arch_bits):
)


@pytest.fixture(params=[True, False], ids=["single", "pool"])
@pytest.fixture(
params=[
(True, PythonParser),
(False, PythonParser),
pytest.param(
(True, HiredisParser),
marks=pytest.mark.skipif(
not HIREDIS_AVAILABLE, reason="hiredis is not installed"
),
),
pytest.param(
(False, HiredisParser),
marks=pytest.mark.skipif(
not HIREDIS_AVAILABLE, reason="hiredis is not installed"
),
),
],
ids=[
"single-python-parser",
"pool-python-parser",
"single-hiredis",
"pool-hiredis",
],
)
def create_redis(request, event_loop):
"""Wrapper around aioredis.create_redis."""
single_connection = request.param
single_connection, parser_cls = request.param

async def f(url: str = request.config.getoption("--redis-url"), **kwargs):
single = kwargs.pop("single_connection_client", False) or single_connection
parser_class = kwargs.pop("parser_class", None) or parser_cls
url_options = parse_url(url)
url_options.update(kwargs)
pool = aioredis.ConnectionPool(**url_options)
pool = aioredis.ConnectionPool(parser_class=parser_class, **url_options)
client: aioredis.Redis = aioredis.Redis(connection_pool=pool)
if single:
client = client.client()
Expand Down
12 changes: 5 additions & 7 deletions tests/test_connection.py
Expand Up @@ -4,17 +4,15 @@

import pytest

from aioredis.connection import UnixDomainSocketConnection
from aioredis.connection import PythonParser, UnixDomainSocketConnection
from aioredis.exceptions import InvalidResponse
from aioredis.utils import HIREDIS_AVAILABLE

if TYPE_CHECKING:
from aioredis.connection import PythonParser


@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
@pytest.mark.asyncio
async def test_invalid_response(r):
@pytest.mark.parametrize("create_redis", [(True, PythonParser)], indirect=True)
async def test_invalid_response(create_redis):
Copy link
Contributor Author

@m-novikov m-novikov Nov 20, 2021

Choose a reason for hiding this comment

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

This test seems like it wasn't run for some time, as in the case of connection pool it will fail with AttributeError, because only a single connection client has a connection set. Ref #1114

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it also didn't work on py =< 3.7

Copy link
Collaborator

Choose a reason for hiding this comment

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

And it also didn't work on py =< 3.7

We should probably drop Python 3.6 now anyways since it's EOL soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But for 3.7 it's still a problem and pypy seems to be unhappy about this test also.
AsyncMock solves this problem.

r = await create_redis()

raw = b"x"
parser: "PythonParser" = r.connection._parser
with mock.patch.object(parser._buffer, "readline", return_value=raw):
Expand Down