Conversation
When a fork occurs while the sqlite db is mid-transaction, so sqlite transaction lock remains locked forever in the foree, deadlocking it. This patch protects against this case using os.register_at_fork().
Previous commit was on Windows after testing there too
grantjenks
left a comment
grantjenks
left a comment
There was a problem hiding this comment.
I haven't looked deeply at the changes yet but thanks for the PR. I approved the CI workflow and asked a couple questions.
Co-authored-by: Grant Jenks <grant.jenks@gmail.com>
| _disk_remove = self._disk.remove | ||
| tid = threading.get_ident() | ||
| txn_id = self._txn_id | ||
| _acquireLock() |
There was a problem hiding this comment.
I'm worried that acquiring the lock at the start of _transact and releasing it at the end is holding it for too long. The yield in the middle of the context manager means the lock could be held for a long while. I haven't thought of a specific scenario that makes this problematic but I'm thinking about it.
The fork system call copies all memory but not all threads. Only the currently executing thread is copied to the forked process. So if one thread holds the SQLite transaction lock and another thread forks ... what's the problem? I don't see how the SQLite transaction remains forever locked.
| try: | ||
| _lock.acquire() | ||
| except BaseException: | ||
| _lock.release() |
There was a problem hiding this comment.
Could an exception occur here in which the lock is released twice?
|
|
||
| def _after_at_fork_child_reinit_locks(): | ||
| global _lock | ||
| _lock = threading.RLock() |
There was a problem hiding this comment.
Why re-init the lock in the child? I would expect the child to already have a copy of the lock.
| if thread.is_alive(): | ||
| os.kill(pid, signal.SIGKILL) | ||
| thread.join() | ||
| assert False, "Deadlock detected." |
There was a problem hiding this comment.
Can you add some more comments to explain how this test works? I appreciate the test a lot but am not sure how to follow it.
3 participants
When a fork occurs while the sqlite db is mid-transaction, so sqlite transaction lock remains locked forever in the foree, deadlocking it.
This patch protects against this case using os.register_at_fork().