Repository navigation
Deadlock in pysqlite_connection_dealloc() #86864
Description
| BPO | 42698 |
|---|---|
| Nosy | @erlend-aasland |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2022-03-15.19:58:08.549>
created_at = <Date 2020-12-21.00:44:44.346>
labels = ['type-bug', '3.9', '3.10', '3.11', 'extension-modules', 'library']
title = 'Deadlock in pysqlite_connection_dealloc()'
updated_at = <Date 2022-03-15.19:58:08.549>
user = 'https://bugs.python.org/hydroflask'bugs.python.org fields:
activity = <Date 2022-03-15.19:58:08.549>
actor = 'hydroflask'
assignee = 'none'
closed = True
closed_date = <Date 2022-03-15.19:58:08.549>
closer = 'hydroflask'
components = ['Extension Modules', 'Library (Lib)']
creation = <Date 2020-12-21.00:44:44.346>
creator = 'hydroflask'
dependencies = []
files = ['50192']
hgrepos = []
issue_num = 42698
keywords = []
message_count = 13.0
messages = ['383471', '383472', '383482', '383489', '398543', '398594', '398801', '398802', '398803', '398804', '413052', '413053', '415289']
nosy_count = 2.0
nosy_names = ['hydroflask', 'erlendaasland']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42698'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']
Activity
hydroflask commentedon Dec 21, 2020
pysqlite_connection_dealloc() calls sqlite3_close{,_v2}(). This can cause a deadlock if sqlite3_close() tries to acquire a lock that another thread holds, due to a deadlock between the GIL and an internal sqlite3 lock.
This is especially common with sqlite3's "shared cache mode."
Since the GIL should not be released during a tp_dealloc function and python has no control over the behavior of sqlite3_close(), it is incorrect to call sqlite3_close() in pysqlite_connection_dealloc().
hydroflask commentedon Dec 21, 2020
This is also a problem in pysqlite_connection_close() as currently implemented.
hydroflask commentedon Dec 21, 2020
Nevermind it seems that it's legal to call Py_BEGIN_ALLOW_THREADS in tp_dealloc. The fix is then to allow threads around sqlite3_close().
hydroflask commentedon Dec 21, 2020
Another comment: if calling sqlite3_close() outside of GIL, then the associated SQL function destructors must take the GIL before calling Py_DECREF
erlend-aasland commentedon Jul 30, 2021
hydroflask, can you provide a reproducer?
hydroflask commentedon Jul 30, 2021
Unfortunately I am currently unable to write a repro for this bug but I am 100% certain it exists from analyzing the code. In our application (which is a large multithreaded application) after I made the changes consistent with this report the deadlock vanished completely. When the deadlock occurred, the thread stacks were consistent with the information in this report.
To get the deadlock to trigger, SQLite itself must take a lock on sqlite3_close() but I don't know very much about SQLite's internals to consistently provoke that behavior. I've attached my attempt at a repro which is similar enough to the pattern of our application but it was not working.
erlend-aasland commentedon Aug 2, 2021
Thanks!
I had to comment out the reg_func() call (it is not defined in test.py, and I don't know what it's supposed to do), and change "with lock.shared_context():" to "with lock:", but I'm unable get a deadlock. I've tried with Python 3.8 though 3.11 (dev) and SQLite 3.7.15 through 3.37.0 (dev).
hydroflask commentedon Aug 2, 2021
You did the right thing in terms of editing the code.
I was not able to get a deadlock with this code which I think should be representative of the error. In production the error was happening after 30 minutes of so.
The major problem is that I don't exactly know how to provoke SQLite to acquire an internal lock. If we assume that it does acquire internal locks and other threads release the GIL before calling into SQLite functions that acquire an internal lock, then you can reason that if sqlite3_close() acquires that lock without first releasing the GIL a deadlock will certainly occur.
This is the deadlock that my application was running into.
erlend-aasland commentedon Aug 2, 2021
Great, thanks.
IIRC, you can provoke the internal SQLite lock simply by using transaction control: BEGIN (lock) => COMMIT / ROLLBACK (unlock).
True. For the record, I'm not doubting the existence of this deadlock, nor the correctness of the proposed solution (allow threads around sqlite3_close()) :)
I'll see if I can come up with a compact repro.
hydroflask commentedon Aug 2, 2021
Ah okay, so the sequence would have to be this:
thread1: pysqlite.some_operation()
thread1: release gil
thread1: sqlite3_some_procedure()
thread1: acquire sqlite lock
<switch threads>
thread2: acquire gil
thread2: pysqlite.close()
thread2: sqlite3_close()
thread2: acquire sqlite lock
It should be possible, good luck!
hydroflask commentedon Feb 11, 2022
Any update on this? I know you wanted to repro but even in the absence of the repro, I think calling sqlite3_close() without releasing the GIL is error-prone. If there is no immediate plan to make this change you may close the issue :)
erlend-aasland commentedon Feb 11, 2022
I was unable to reproduce this last time we visited this issue, but I'll give it another try.
I do not want to close it until I can a) either prove that it is _not_ an issue, or b) prove that it _is_ an issue _and_ we've got a fix :)
hydroflask commentedon Mar 15, 2022
Closing this issue since sqlite3_close{,_v2}() is no longer called with the GIL held in 3.11+