search

redis / redis-py

Redis Python client
MIT License
12.57k stars 2.51k forks source link

Socket error vs. OSError #3294

Closed dinoDayo closed 3 months ago

dinoDayo commented 3 months ago

Reference: https://github.com/redis/redis-py/blob/b7f9a4c02aeb687d4db307a63cff0fb24aaec427/redis/asyncio/connection.py#L457

TL;DR: This is a continuation of a known bug in the sync implementation of this library. I think a corresponding edit is needed for the async function definition.

I believe an additional except socket.error clause may needed in the async definition of self.disconnect. The synchronous version of the self.disconnect function had that clause added after users started reporting seeing <AbstractConnection.__del__ at HEXMEMORYLOC error messages. See conversation and resolution linked here. I have been seeing the same errors in my asynchronous implementation of this library and suspect that a PR that adds: 

        except socket.error:
            pass

in line 457 of https://github.com/redis/redis-py/blob/b7f9a4c02aeb687d4db307a63cff0fb24aaec427/redis/asyncio/connection.py should address these issues. I will test locally and update once I am sure it works for my use-case.

Thank you for your time!

dinoDayo commented 3 months ago

I tried creating a branch and making a PR but I got a 403 response. Will update once I am sure this fix works for us, it has been deployed but will take a few days for me to be sure.

dinoDayo commented 3 months ago

Following up - applying this patch led to more downstream errors. These errors were helpful context into their root cause; celery does not explicitly close connections with the redis backend, but rather relies on the __del__ method during interpreter shutdown. For anyone facing similar issues, check this thread out for potential next steps in debugging. Will update if/when a stable solution is identified for my use-case.