KeyError on self._in_use_connections.remove(connection)

[jkamashian]

Version: 2.10.5
Platform: Python 2.7, Ubuntu
This is a very rare occurrence on a very high traffic server but I receive a key error occasionally from here:
self._in_use_connections.remove(connection)
https://github.com/andymccurdy/redis-py/blob/2.10.5/redis/connection.py#L913
I haven't been able to reproduce this in testing or root out the cause.

Theories:
self._available_connections is a list so you could append in the same connection more then once. Then when you try to self._in_use_connections.add(connection)
https://github.com/andymccurdy/redis-py/blob/2.10.5/redis/connection.py#L898
You get no error for using the same connection. Until you try to release _in_use_connections is a set so were the add() multiple times desen't raise an error remove() will.

[andymccurdy]

Can you elaborate on your setup a little more? Are you using redis-py in a forked or multiprocess or threaded environment?

[jkamashian]

So this is a Flask(Nginx+) I think threaded Is the correct answer for this.

[Yanght115]

I have the same problem with python 2.7 + Django + uwsgi.

class RedisManager(Singleton):
    redis_conf = settings.BASE_PLUGIN_REDIS_CONFIG
    info_connection_pool = redis.ConnectionPool(
        host=redis_conf['info']['host'], port=redis_conf['info']['port'], decode_responses=True, db=0)

    @property
    def info_client(self):
        return redis.Redis(connection_pool=self.info_connection_pool)

Sometimes I get the following error

  File "/media/disk1/fordata/web_server/project/paas-pcs-platform/1.0.796-preonline/bem/base_plugin/util.py", line 62, in __init__
    self._init_redis_connection_pool()
  File "/media/disk1/fordata/web_server/project/paas-pcs-platform/1.0.796-preonline/bem/base_plugin/util.py", line 67, in _init_redis_connection_pool
    assert redis.Redis(connection_pool=self.status_connection_pool).ping(
  File "/data/web_server/project/paas-pcs-platform/venv/lib/python2.7/site-packages/redis/client.py", line 1037, in ping
    return self.execute_command('PING')
  File "/data/web_server/project/paas-pcs-platform/venv/lib/python2.7/site-packages/redis/client.py", line 784, in execute_command
    pool.release(connection)
  File "/data/web_server/project/paas-pcs-platform/venv/lib/python2.7/site-packages/redis/connection.py", line 1032, in release
    self._in_use_connections.remove(connection)
KeyError: Connection<host=xxxxx,port=xxxxx,db=0>

I guess the problem is caused by the Shared connection pool used by uwsgi workers ？

[kmerenkov]

We get the same issue. Forked environment, all connections are made after fork.

[andymccurdy]

@kmerenkov What version of redis-py are you using? 3.2.0+ fixes a lot of the problems with connections in forked processes.

[kmerenkov]

What version of redis-py are you using?

3.2.1

[Yanght115]

 def reset(self):
        self.pid = os.getpid()
        self._created_connections = 0
        self._available_connections = []
        self._in_use_connections = set()
        self._check_lock = threading.Lock()

    def _checkpid(self):
        if self.pid != os.getpid():
            with self._check_lock:
                if self.pid == os.getpid():
                    # another thread already did the work while we waited
                    # on the lock.
                    return
                self.reset()

    def get_connection(self, command_name, *keys, **options):
        "Get a connection from the pool"
        self._checkpid()
        try:
            connection = self._available_connections.pop()
        except IndexError:
            connection = self.make_connection()

I guess there's one case, thread A and B, A get lock and then call reset(), when A set pid with self.pid = os.getpid() but not self._in_use_connections = set(), B enter the _checkpid() and fail to meet the condition if self.pid != os.getpid(), so B return and get connection from old _available_connections

[gmbnomis]

I have the same issue (on 3.3.11).

I have no idea if this is the correct fix, but based on @yht804421715's observation, I moved the update of self.pid in reset() to the end:

    def reset(self):
        self._created_connections = 0
        self._available_connections = []
        self._in_use_connections = set()
        self._check_lock = threading.Lock()
        self.pid = os.getpid()

Since then, I can't reproduce it anymore.

[mdellweg]

@gmbnomis without fully understanding what is happening here, i think you should not move the pid-updating behind the discarding of the lock. From the comment i read that by setting the pid, the thread communicates, that is has done the work. So moving it behind clearing the containers is ok imho.
Anyway, i don't understand why the old lock should be discarded at all. Releasing it should be enough (and safer).

[andymccurdy]

I'm not a fan of simply moving the .pid assignment below the other statements. It might work more than the released version but it's still not thread safe.

I just created a new PR #1270 that implements what I believe is a fully thread-safe pool. Can anyone please give it a spin and see if it fixes your issue? Thanks!