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

Connection is not thread-safe #1178

Closed
gloryfromca opened this issue Jun 20, 2019 · 2 comments · Fixed by #1270
Closed

Connection is not thread-safe #1178

gloryfromca opened this issue Jun 20, 2019 · 2 comments · Fixed by #1270

Comments

@gloryfromca
Copy link

gloryfromca commented Jun 20, 2019

Version: redis==2.10.6

Platform: Python 3.7 on Ubuntu 18.04

Description:
I encountered the same question with the link: celery/celery#4363

I reproduce the error by inserting time.sleep() in Connection.connect which is trying to initialize self._sock, then start two thread, make both pass self._sock existence check and run into Connection._on_connect (apprently Connection.connnect without any thread-lock).

So when one thread in on_connect() call read_response, it maybe get another thread's response, then raise the error.

def on_connect(self):
    "Initialize the connection, authenticate and select a database"
    self._parser.on_connect(self)

    # if a password is specified, authenticate
    if self.password:
        self.send_command('AUTH', self.password)
        if nativestr(self.read_response()) != 'OK':
            raise AuthenticationError('Invalid Password')

    # if a database is specified, switch to it
    if self.db:
        self.send_command('SELECT', self.db)
        res = self.read_response()
        if nativestr(res) != 'OK':
            raise ConnectionError('Invalid Database')

Is this a bug or something I don't get?

@andymccurdy
Copy link
Contributor

Connections are not meant to be threadsafe. ConnectionPools are threadsafe.

@wynemo
Copy link

wynemo commented Nov 4, 2019

@andymccurdy #906 it seems the default connectionpool is not threading safe.

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.

3 participants