Make database connection thread-safe #6030
Conversation
CommonData/databaseinterface.cpp
Outdated
| sqlite3* connection = _dbs.at(std::this_thread::get_id()); | ||
| _loadMutex.unlock(); | ||
| return; | ||
| return connection; |
There was a problem hiding this comment.
Why are you returning this?
CommonData/databaseinterface.h
Outdated
|
|
||
| void create(); ///< Creates a new sqlite database in sessiondir and loads it | ||
| void load(); ///< Loads a sqlite database from sessiondir (after loading a jaspfile) | ||
| sqlite3* load(); ///< Loads a sqlite database from sessiondir (after loading a jaspfile) |
There was a problem hiding this comment.
I on purpose kept sqlite3 stuff out of the api because this is supposed to abstract away from that so we could change sqlite for some other sql some day.
Why did you add it?
There was a problem hiding this comment.
That's true, this is not necessary anymore (this is just a relic of old attempts). I'll change this
| if(!_dbs.count(id)) | ||
| load(); | ||
|
|
||
| return _dbs.at(id); |
There was a problem hiding this comment.
@RensDofferhoff and me thought also reading in a map is not really thread-safe: a map has no mutex, so if a thread add an element to the map at the same moment that count or at is called, it cannot guarantee the right value.
But as for the moment only one extra thread (the _loaderThread from MainWindow) is used, the map will have in fact only 1 value (the other connection is set in _dbCreated. So that's ok for now...
There was a problem hiding this comment.
Which is why I put mutexes in the load and creation function...
There was a problem hiding this comment.
And there are waaaaay more threads being used so im not sure why you think there is only one?
There was a problem hiding this comment.
class InitColumnTask : public QRunnable look for that
There was a problem hiding this comment.
It doesn't matter you only guard against concurrent writes into the map with that mutex.
Reads can happen during load and creation.
count() and at() of std::map are not safe when the map is not const.
There was a problem hiding this comment.
But then reading in the map should also be enclosed by the mutex.
There was a problem hiding this comment.
So we still have that potential problem then you mean?
There was a problem hiding this comment.
But then reading in the map should also be enclosed by the mutex.
Yes.
There was a problem hiding this comment.
well Ill add it to the PR im opening in a few minutes then!
* for ordinal and nominal columns not the labelid but the label as shown to the user determines whether it is a level or not fixes jasp-stats/jasp-issues#3721 * follow the good advice of Rens and Bruno and adding a mutex advice is here: #6030 * lock more carefully
3 participants
When working on the new feature to generate reports with feeding jasp files with other datafiles (#6025), I came upon an issue that sometimes the synchronization hangs.
This was due to the fact that after opening a JASP file, the Filter model generates some SQL statements, but the synchronization task was already started: this lead to concurrency issues with the database. Especially the connection object to SQLite is thread dependent, but
_transactionWriteDepthand_transactionReadDepthare not. So one thread could increment _transactionWriteDepth, and another thread checking _transactionWriteDepth would think that a transaction was already started, and then would not execute a 'BEGIN EXCLUSIVE' statement.So the solution is to make
_transactionWriteDepthand_transactionReadDepththread dependent.Also the connection object is stored in a map, but a std map does not use mutex: so setting and querying the map must be done under a mutex to be sure that the manipulation of the map is thread safe.
This fix solves probably some hanging issues that some users report.