PYTHON-2484 Added lock sanitization for MongoClient and ObjectId #985
Conversation
|
Currently fails on an pymongocrypt error that doesn't appear to be related. |
bson/__init__.py
Outdated
|
|
||
|
|
||
| def _after_fork(): | ||
| """Releases the ObjectID lock in parent and child.""" |
There was a problem hiding this comment.
"in parent and child" -> "in child"
pymongo/mongo_client.py
Outdated
| @@ -846,7 +857,11 @@ def target(): | |||
| from pymongo.encryption import _Encrypter | |||
|
|
|||
| self._encrypter = _Encrypter(self, self.__options.auto_encryption_opts) | |||
| self._timeout = options.timeout | |||
| self._timeout = self.__options.timeout | |||
There was a problem hiding this comment.
I suggest moving this section out of _init_deadlockables since it only needs to happen once in __init__, not after a fork:
if connect:
self._get_topology()
self._encrypter = None
if self.__options.auto_encryption_opts:
from pymongo.encryption import _Encrypter
self._encrypter = _Encrypter(self, self.__options.auto_encryption_opts)
self._timeout = options.timeout
pymongo/mongo_client.py
Outdated
| @@ -817,6 +821,14 @@ def __init__( | |||
| srv_max_hosts=srv_max_hosts, | |||
| ) | |||
|
|
|||
| self._init_deadlockables(connect) | |||
There was a problem hiding this comment.
Suggest renaming to self._init_sdam or self._init_background
pymongo/mongo_client.py
Outdated
| @@ -832,7 +844,6 @@ def target(): | |||
| target=target, | |||
| name="pymongo_kill_cursors_thread", | |||
| ) | |||
|
|
|||
pymongo/lock.py
Outdated
| # References to instances of _create_lock | ||
| _forkable_locks: weakref.WeakSet = weakref.WeakSet() | ||
|
|
||
| _insertion_lock = threading.Lock() |
There was a problem hiding this comment.
WeakSet.add is implementing using set.add so it is threadsafe and we can remove _insertion_lock and _clients_lock:
https://github.com/python/cpython/blob/3.7/Lib/_weakrefset.py#L81-L84
There was a problem hiding this comment.
Even asyncio uses WeakSet.add from multiple threads without a lock:
https://github.com/python/cpython/blob/3.7/Lib/asyncio/tasks.py#L860
test/test_fork.py
Outdated
|
|
||
| def exit_cond(): | ||
| with _insertion_lock: | ||
| return 0 # success |
There was a problem hiding this comment.
It might be better to run an operation on the client in the child process here, like:
self.client.admin.command('ping')
pymongo/lock.py
Outdated
| return lock | ||
|
|
||
|
|
||
| def _release_locks(child: bool) -> None: |
pymongo/mongo_client.py
Outdated
| # This will run in the same thread as the fork was called. | ||
| # If we fork in a critical region on the same thread, it should break. | ||
| # This is fine since we would never call fork directly from a critical region. | ||
| os.register_at_fork(after_in_child=_after_fork_child, after_in_parent=_after_fork_parent) |
There was a problem hiding this comment.
after_in_parent is no longer needed.
Adds some
_after_fork()calls to reset locks forObjectIdandMongoClient, and some unit tests for both classes.