Skip to content
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

Off by 1 - Canceling async Redis command leaves connection open, in unsafe state for future commands #2624

Closed
drago-balto opened this issue Mar 17, 2023 · 5 comments · Fixed by #2641

Comments

@drago-balto
Copy link

drago-balto commented Mar 17, 2023

Version: 4.5.1

Platform: Python 3.8 / Ubuntu (but really, any platform will likely suffer the same issue)

Description: Canceling async Redis command leaves connection open, in unsafe state for future commands

The issue here is really the same as #2579, except that it generalizes it to all commands (as opposed to just blocking commands).

If async Redis request is canceled at the right time, after the command was sent but before the response was received and parsed, the connection is left in an unsafe state for future commands. The following redis operation on the same connection will send the command, and then promptly continue to read the response from the previous, canceled command. From that point on, the connection will remain in this weird, off-by-1 state.

Here's a script reproducing the problem:

import asyncio
from redis.asyncio import Redis
import sys


async def main():
    myhost, mypassword = sys.argv[1:]
    async with Redis(host=myhost, password=mypassword, ssl=True, single_connection_client=True) as r:

        await r.set('foo', 'foo')
        await r.set('bar', 'bar')

        t = asyncio.create_task(r.get('foo'))
        await asyncio.sleep(0.001)
        t.cancel()
        try:
            await t
            print('try again, we did not cancel the task in time')
        except asyncio.CancelledError:
            print('managed to cancel the task, connection is left open with unread response')

        print('bar:', await r.get('bar'))
        print('ping:', await r.ping())
        print('foo:', await r.get('foo'))


if __name__ == '__main__':
    asyncio.run(main())

Running this against a server in the cloud (such that requests typically take > 5ms to complete) results in this:

$ python redis_cancel.py hostname password
managed to cancel the task, connection is left open with unread response
bar: b'foo'
ping: False
foo: b'PONG'

I believe the solution is simple. This method needs to be modified to disconnect connection when current request is canceled. For example, do this:

    async def _send_command_parse_response(self, conn, command_name, *args, **options):
        """
        Send a command and parse the response
        """
        try:
            await conn.send_command(*args)
            return await self.parse_response(conn, command_name, **options)
        except asyncio.CancelledError:
            await conn.disconnect()
            raise
@volnet
Copy link

volnet commented Mar 24, 2023

Is it the problem about OpenAI https://openai.com/blog/march-20-chatgpt-outage ?

@drago-balto
Copy link
Author

Yep, that's the one, and the #2641 has not fixed it fully, as I already commented here: #2641 (comment)

I am asking for this ticket to be re-oped, since I can still reproduce the problem in the latest 4.5.3. version

@rajivpoddar
Copy link

Did GPT4 help with debugging?

@pydanny
Copy link

pydanny commented Mar 24, 2023

I wonder how much OpenAI/ChatGPT is funding maintenance of this library. 🤔

@chayim
Copy link
Collaborator

chayim commented Mar 26, 2023

Folks, I reopened this accidentally - but wanted to ensure there was communication that is clear. Please note - we're tracking in #2665. Let's align in a single issue - it makes it easier to co-ordinate, track, and respond.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants