-
Notifications
You must be signed in to change notification settings - Fork 25
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
PersistentDict: thread safety #234
Conversation
c8adf8e
to
6cecb1d
Compare
6cecb1d
to
07412b2
Compare
pytools/__init__.py
Outdated
SQLite-related functions | ||
------------------------ | ||
|
||
.. autofunction:: get_sqlite3_thread_safety_level | ||
.. autofunction:: is_sqlite3_fully_threadsafe |
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'm not sure I'd like to document these, since it encodes things that feel like implementation details. Maybe push this off into pytools._sqlite
?
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.
Moving these functions into a separate module makes sense I think, but I thought these functions are pretty generic? Which implementation details do they encode?
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.
How cpython and threads and sqlite interact. The details of how this works at the moment is kind of insane ATM, and I try to avoid insanity in publicly exposed interfaces. Hence my preference to keep this out of public view.
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.
How cpython and threads and sqlite interact. The details of how this works at the moment is kind of insane ATM
I see - is this in reference to the translation between thread support levels in 07412b2#diff-2d67e0c2f525bd7f05e4909f1248231d4b1f3b92d2f1be94487d5b2d4cb46b78R2992-R2996, or are there other problems in interacting between threads/sqlite? If so, could those be the issue for the failures I mentioned in #234 (comment)?
Another idea: in case the sqlite library is not built with thread safety, can we just use a mutex every time we use the connection (e.g., in |
704e26c
to
7274976
Compare
d935763
to
4ca16cc
Compare
It seems it is crashing in the multithreaded case where the connection is shared among threads, a situation which should not crash even without the mutex :-(
|
This reverts commit bead987.
Well, that turned out to be more ... interesting... than expected. The final error only happened with Python 3.12, which made debugging this more difficult. This is ready for testing/review. @vincefn: Could you test this PR against the failure you had mentioned in #233? It does avoid the warning I had seen in the boxtree test in #233. |
Every sql command needs to be wrapped in _exec_sql, for the 'spin while busy' functionality
Sorry it took a while - I could only reproduce that inside a gitlab CI test... But the tests which failed are now passing with this fix :-) |
Workaround for #233.
cc: illinois-ceesd/logpyle#106 illinois-ceesd/logpyle#107 illinois-ceesd/logpyle#108
Based partly on https://ricardoanderegg.com/posts/python-sqlite-thread-safety/.
Notes:
The only other alternative I can see is to use a fully multithreaded implementation such as https://github.com/piskvorky/sqlitedict