Commit 145caeac authored by Stefan Brüns's avatar Stefan Brüns

[Bookmarks Runner] Open database connection in the query thread

Summary:
QSqlDatabase connection instances are global reference counted objects which
can be crated using the QSqlDatabase::addDatabase(...) method and later
retrieved with QSqlDatabase::database(connectionname).

Connections should only be used from a single thread, and should be cleaned
up with QSD::removeDatabase(name), which implicitly closes the connection
when the last reference is removed.

Currently, the same connection (name) is reused over all threads, and
the connection is only removed implicitly by replacing it via addDatabase().
This leads to warnings, i.e.: "QSqlDatabasePrivate::addDatabase: duplicate
connection name '...', old connection removed."

As one reference is held by the m_db member, the implicit removal triggers
another warning: "QSqlDatabasePrivate::removeDatabase: connection '..."
is still in use, all queries will cease to work."

According to the documentation, "It is highly recommended that you do not
keep a copy of the QSqlDatabase around as a member of a class, as this
will prevent the instance from being correctly cleaned up on shutdown."
There is no need to keep a reference using a member variable, as retrieving
an open connection via QSqlDatabase::database(...) is cheap.

Create a per-thread DB connection on first use, and remove the connections
when the FetchSqlite instance is torn down.

Delaying the open is beneficial for the favicon instance, which may be
unused when the icon is already cached on disk.

See also T9626.

BUG: 400723

Test Plan:
execute a query in krunner
1. Results are as expected
2. The 'QSqlDatabasePrivate::removeDatabase: connection ... is still in use,
   all queries will cease to work.' warning no longer appears
3. The 'QSqlDatabasePrivate::addDatabase: duplicate connection name ...,
   old connection removed' warning no longer appears

Reviewers: #frameworks, mart

Reviewed By: mart

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D16409
parent 22545a3f
......@@ -21,18 +21,18 @@
#include "fetchsqlite.h"
#include <QFile>
#include <QDebug>
#include "bookmarks_debug.h"
#include "bookmarksrunner_defs.h"
#include <QSqlQuery>
#include <QSqlError>
#include <QSqlRecord>
#include <QMutexLocker>
#include <thread>
#include <sstream>
FetchSqlite::FetchSqlite(const QString &originalFilePath, const QString &copyTo, QObject *parent) :
QObject(parent), m_databaseFile(copyTo)
{
m_db = QSqlDatabase::addDatabase(QStringLiteral("QSQLITE"), originalFilePath);
m_db.setHostName(QStringLiteral("localhost"));
QFile originalFile(originalFilePath);
QFile(copyTo).remove();
bool couldCopy = originalFile.copy(copyTo);
......@@ -44,33 +44,58 @@ FetchSqlite::FetchSqlite(const QString &originalFilePath, const QString &copyTo,
FetchSqlite::~FetchSqlite()
{
if(m_db.isOpen()) m_db.close();
QFile(m_databaseFile).remove();
}
void FetchSqlite::prepare()
{
QMutexLocker lock(&m_mutex);
m_db.setDatabaseName(m_databaseFile);
bool ok = m_db.open();
//qDebug() << "Sqlite Database " << m_databaseFile << " was opened: " << ok;
if(!ok) {
//qDebug() << "Error: " << m_db.lastError().text();
}
}
void FetchSqlite::teardown()
{
QMutexLocker lock(&m_mutex);
m_db.close();
QString connectionPrefix = m_databaseFile + "-";
const auto connections = QSqlDatabase::connectionNames();
for (auto c : connections) {
if (c.startsWith(connectionPrefix)) {
qCDebug(RUNNER_BOOKMARKS) << "Closing connection" << c;
QSqlDatabase::removeDatabase(c);
}
}
}
static QSqlDatabase openDbConnection(const QString& databaseFile) {
// create a thread unique connection name based on the DB filename and thread id
auto connection = databaseFile + "-";
std::stringstream s;
s << std::this_thread::get_id();
connection += QString::fromStdString(s.str());
// Try to reuse the previous connection
auto db = QSqlDatabase::database(connection);
if (db.isValid()) {
qCDebug(RUNNER_BOOKMARKS) << "Reusing connection" << connection;
return db;
}
// Otherwise, create, configure and open a new one
db = QSqlDatabase::addDatabase(QStringLiteral("QSQLITE"), connection);
db.setHostName(QStringLiteral("localhost"));
db.setDatabaseName(databaseFile);
db.open();
qCDebug(RUNNER_BOOKMARKS) << "Opened connection" << connection;
return db;
}
QList<QVariantMap> FetchSqlite::query(const QString &sql, QMap<QString, QVariant> bindObjects)
{
QMutexLocker lock(&m_mutex);
auto db = openDbConnection(m_databaseFile);
//qDebug() << "query: " << sql;
QSqlQuery query(m_db);
QSqlQuery query(db);
query.prepare(sql);
foreach(const QString &variableName, bindObjects.keys()) {
query.bindValue(variableName, bindObjects.value(variableName));
......@@ -78,7 +103,7 @@ QList<QVariantMap> FetchSqlite::query(const QString &sql, QMap<QString, QVariant
}
if(!query.exec()) {
QSqlError error = m_db.lastError();
QSqlError error = db.lastError();
//qDebug() << "query failed: " << error.text() << " (" << error.type() << ", " << error.number() << ")";
//qDebug() << query.lastQuery();
}
......@@ -93,6 +118,7 @@ QList<QVariantMap> FetchSqlite::query(const QString &sql, QMap<QString, QVariant
}
result << recordValues;
}
return result;
}
......@@ -100,5 +126,6 @@ QStringList FetchSqlite::tables(QSql::TableType type)
{
QMutexLocker lock(&m_mutex);
return m_db.tables(type);
auto db = openDbConnection(m_databaseFile);
return db.tables(type);
}
......@@ -46,7 +46,6 @@ public:
private:
QString const m_databaseFile;
QMutex m_mutex;
QSqlDatabase m_db;
};
#endif // FETCHSQLITE_H
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment