-
Notifications
You must be signed in to change notification settings - Fork 27
INTPYTHON-574 Add support for connection pooling #290
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
Conversation
2037b16
to
8178344
Compare
8178344
to
10b2fe3
Compare
# setdefault() ensures that multiple threads don't set this in | ||
# parallel. | ||
self._connection_pools.setdefault(self.alias, conn) | ||
return self._connection_pools[self.alias] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ever possible for this cache to become out of sync? Does the user ever have raw access to the MongoClient (where they could call client.close()
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the MonogClient
is available at django.db.connection.connection
, but I don't think it's our job to consider situations where the user does something unexpected like calling close()
.
@@ -176,7 +177,12 @@ def get_connection_params(self): | |||
|
|||
@async_unsafe | |||
def get_new_connection(self, conn_params): | |||
return MongoClient(**conn_params, driver=self._driver_info()) | |||
if self.alias not in self._connection_pools: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Django (or Django apps) commonly use fork() or multiprocessing? If so we should consider clearing this cache in the child process. Perhaps using
if hasattr(os, "register_at_fork"):
os.register_at_fork(after_in_child=clear_client_cache)
https://docs.python.org/3/library/os.html#os.register_at_fork:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not something I've had experience with. Google AI overview says:
Django, when deployed using WSGI servers like Gunicorn or uWSGI, operates in a prefork model. This means the server forks multiple worker processes to handle incoming requests concurrently. Each worker process runs independently, allowing Django to manage multiple requests simultaneously. However, directly using os.fork within a Django application is generally discouraged due to potential conflicts with Django's request handling and database connections. For managing background tasks or parallel processing, libraries like multiprocessing or task queues such as Celery are recommended instead of direct forking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sounds good to me. We can reevaluate if someone reports it as a problem.
django_mongodb_backend/base.py
Outdated
def close(self): | ||
super().close() | ||
# MongoClient is a connection pool and, unlike database drivers that | ||
# implement PEP 249, shouldn't be closed by connection.close(). | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overriding close()
to do nothing omits the call to validate_thread_sharing()
in the superclass implementation. If it's important, we could add that here. There's currently a test failure without it.
Threading, connection sharing between threads... it's all not much I have experience with, so I'm not too confident about any of this. Perhaps you can help to educate me if any of it is obvious to you, Shane.
django/django@34e248e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would skip those threaded tests since pymongo does not implement PEP 249. We intentionally want to always share the same client among all threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the client is already shared among threads as it's stored in the DatabaseWrapper._connection_pools
class variable.
The validate_thread_sharing()
logic ensures that separate instances of DatabaseWrapper
aren't accessed in separate threads. The use case for disabling this is to allow in-memory sqlite database connections being shared between multiple threads for Selenium tests (#2879). I'm doubtful that disabling this logic for MongoDB is appropriate, so unless you can say definitively, I'd suggest we leave it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the answer to this.
bde56c5
to
98b937c
Compare
django_mongodb_backend/base.py
Outdated
def _close(self): | ||
pass | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What function calls this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally DatabaseWrapper.close()
but it's also called by some tests.
98b937c
to
2cd94a5
Compare
2cd94a5
to
a643d98
Compare
Some inspiration taken from the implementation of PostgreSQL's connection pooling.