fixed cleanup code of engine threads

Author: Skycoder42

src/datasync/conflictresolver.cpp

@@ -1,5 +1,6 @@
 #include "conflictresolver.h"
 #include "conflictresolver_p.h"
+#include "defaults_p.h"
 using namespace QtDataSync;
 
 ConflictResolver::ConflictResolver(QObject *parent) :
@@ -11,9 +12,9 @@ ConflictResolver::~ConflictResolver() = default;
 
 void ConflictResolver::setDefaults(const Defaults &defaults)
 {
-	d->defaults = defaults;
-	d->logger = d->defaults.createLogger(name(), this);
-	d->settings = d->defaults.createSettings(this, QString::fromUtf8(name()));
+	d->defaultsName = defaults.setupName();
+	d->logger = defaults.createLogger(name(), this);
+	d->settings = defaults.createSettings(this, QString::fromUtf8(name()));
 }
 
 QByteArray ConflictResolver::name() const
@@ -23,7 +24,7 @@ QByteArray ConflictResolver::name() const
 
 Defaults ConflictResolver::defaults() const
 {
-	return d->defaults;
+	return DefaultsPrivate::obtainDefaults(d->defaultsName);
 }
 
 Logger *ConflictResolver::logger() const

src/datasync/conflictresolver_p.h

@@ -10,7 +10,7 @@ namespace QtDataSync {
 class ConflictResolverPrivate
 {
 public:
-	Defaults defaults;
+	QString defaultsName;
 	Logger *logger = nullptr;
 	QSettings *settings = nullptr;
 };

src/datasync/defaults.cpp

@@ -219,7 +219,6 @@ void DefaultsPrivate::createDefaults(const QString &setupName, bool isPassive, c
 void DefaultsPrivate::removeDefaults(const QString &setupName)
 {
 	QMutexLocker _(&setupDefaultsMutex);
-#ifndef QT_NO_DEBUG
 	QWeakPointer<DefaultsPrivate> weakRef;
 	{
 		auto ref = setupDefaults.take(setupName);
@@ -233,30 +232,6 @@ void DefaultsPrivate::removeDefaults(const QString &setupName)
 #undef QTDATASYNC_LOG
 #define QTDATASYNC_LOG logger
 	}
-#else
-	setupDefaults.remove(setupName);
-#endif
-}
-
-void DefaultsPrivate::clearDefaults()
-{
-	QMutexLocker _(&setupDefaultsMutex);
-#ifndef QT_NO_DEBUG
-	QList<QPair<QString, QWeakPointer<DefaultsPrivate>>> weakRefs;
-	for(auto it = setupDefaults.constBegin(); it != setupDefaults.constEnd(); it++)
-		weakRefs.append({it.key(), it.value().toWeakRef()});
-	setupDefaults.clear();
-	for(const auto &ref : qAsConst(weakRefs)) {
-#undef QTDATASYNC_LOG
-#define QTDATASYNC_LOG ref.second.toStrongRef()->logger
-		if(ref.second)
-			logCritical() << "Defaults for setup still in user after setup was deleted!";
-#undef QTDATASYNC_LOG
-#define QTDATASYNC_LOG logger
-	}
-#else
-	setupDefaults.clear();
-#endif
 }
 
 QSharedPointer<DefaultsPrivate> DefaultsPrivate::obtainDefaults(const QString &setupName)

src/datasync/defaults_p.h

@@ -54,7 +54,6 @@ class Q_DATASYNC_EXPORT DefaultsPrivate : public QObject
 							   QJsonSerializer *serializer,
 							   ConflictResolver *resolver);
 	static void removeDefaults(const QString &setupName);
-	static void clearDefaults();
 	static QSharedPointer<DefaultsPrivate> obtainDefaults(const QString &setupName);
 
 	DefaultsPrivate(QString setupName,

src/datasync/exchangeengine.cpp

@@ -175,7 +175,7 @@ void ExchangeEngine::finalize()
 	//remoteconnector is the only one asynchronous (for now)
 	connect(_remoteConnector, &RemoteConnector::finalized,
 			thread(), &QThread::quit,
-			Qt::DirectConnection); //TODO unsafe?
+			Qt::DirectConnection);
 
 	_syncController->finalize();
 	_changeController->finalize();
@@ -350,16 +350,15 @@ EngineThread::EngineThread(QString setupName, ExchangeEngine *engine, QLockFile
 	setTerminationEnabled(true);
 	_engine.load()->moveToThread(this);
 
-	connect(thread(), &QThread::finished,
-			this, &EngineThread::stopSelf);
-	//TODO dangerous? find better way
-//	connect(this, &QThread::finished,
-//			this, &QThread::deleteLater);
+	connect(this, &QThread::finished,
+			this, [this](){
+		_deleteBlocker.clear();
+	}, Qt::QueuedConnection);
 }
 
 EngineThread::~EngineThread()
 {
-	Q_ASSERT_X(!isRunning(), Q_FUNC_INFO, "Engine thread destroyed while engine is still running");
+	Q_ASSERT_X(isFinished(), Q_FUNC_INFO, "Engine thread destroyed while engine is still running");
 }
 
 QString EngineThread::name() const
@@ -372,11 +371,6 @@ ExchangeEngine *EngineThread::engine() const
 	return _engine.load();
 }
 
-bool EngineThread::isRunning() const
-{
-	return _running;
-}
-
 bool EngineThread::startEngine()
 {
 	if(!_running.testAndSetRelaxed(false, true))
@@ -396,7 +390,7 @@ bool EngineThread::stopEngine()
 
 void EngineThread::waitAndTerminate(unsigned long timeout)
 {
-	if(!isRunning())
+	if(isFinished())
 		return;
 	if(!wait(timeout)) {
 		qCWarning(qdssetup) << "Workerthread of setup" << _name << "did not finish before the timout. Terminating...";
@@ -410,13 +404,21 @@ void EngineThread::waitAndTerminate(unsigned long timeout)
 void EngineThread::run()
 {
 	try {
+		// run
 		qCDebug(qdssetup) << "Engine thread for setup" << _name << "has started";
 		_engine.load()->initialize();
 		exec();
+
+		// cleanup
 		_running = false;
 		delete _engine;
 		_lockFile->unlock();
 		delete _lockFile;
+		DefaultsPrivate::removeDefaults(_name);
+
+		// remove from engine list
+		QMutexLocker _cn(&SetupPrivate::setupMutex);
+		_deleteBlocker = SetupPrivate::engines.take(_name);
 		qCDebug(qdssetup) << "Engine thread for setup" << _name << "has stopped";
 	} catch(QException &e) {
 		if(_lockFile)
@@ -427,9 +429,3 @@ void EngineThread::run()
 			qFatal("%s", e.what());
 	}
 }
-
-void EngineThread::stopSelf()
-{
-	stopEngine();
-	waitAndTerminate(SetupPrivate::currentTimeout());
-}

src/datasync/exchangeengine_p.h

@@ -129,7 +129,6 @@ class EngineThread : public QThread
 
 	QString name() const;
 	ExchangeEngine *engine() const;
-	bool isRunning() const;
 
 	bool startEngine();
 	bool stopEngine();
@@ -139,14 +138,14 @@ class EngineThread : public QThread
 protected:
 	void run() override;
 
-private Q_SLOTS:
-	void stopSelf();
-
 private:
 	const QString _name;
 	QAtomicPointer<ExchangeEngine> _engine;
 	QLockFile *_lockFile;
 	QAtomicInteger<quint16> _running = false;
+
+	//delete blocker
+	QSharedPointer<EngineThread> _deleteBlocker;
 };
 
 }

src/datasync/setup.cpp

@@ -26,15 +26,22 @@ void Setup::setCleanupTimeout(unsigned long timeout)
 void Setup::removeSetup(const QString &name, bool waitForFinished)
 {
 	QMutexLocker _(&SetupPrivate::setupMutex);
+	if(!SetupPrivate::engines.contains(name))
+		return;
 	auto mThread = SetupPrivate::engines.value(name);
 	_.unlock(); //unlock first
 
 	if(mThread) {
 		mThread->stopEngine();
 		if(waitForFinished)
 			mThread->waitAndTerminate(SetupPrivate::timeout);
-	} else //no there -> remove defaults (either already removed does nothing, or remove passive)
+	} else {
+		//was set but null -> must be passive -> remove passive defaults
 		DefaultsPrivate::removeDefaults(name);
+		// and remove from setup
+		_.relock();
+		SetupPrivate::engines.remove(name);
+	}
 }
 
 QStringList Setup::keystoreProviders()
@@ -428,9 +435,6 @@ Setup &Setup::setAccountTrusted(const QByteArray &importData, const QString &pas
 
 void Setup::create(const QString &name)
 {
-	if(QThread::currentThread() != qApp->thread())
-		qCWarning(qdssetup) << "Setup::create should only be called from the main thread";
-
 	QMutexLocker _(&SetupPrivate::setupMutex);
 	if(SetupPrivate::engines.contains(name))
 		throw SetupExistsException(name);
@@ -450,27 +454,29 @@ void Setup::create(const QString &name)
 	if(d->initialImport.isSet())
 		engine->prepareInitialAccount(d->initialImport);
 
-	// create and connect the new thread
-	auto thread = new EngineThread{name, engine, lockFile};
-	// once the thread finished, clear the engine from the cache of known ones
-	QObject::connect(thread, &QThread::finished, [name](){
-		QMutexLocker _cn(&SetupPrivate::setupMutex);
-		SetupPrivate::engines.remove(name);
-		_cn.unlock();
-		DefaultsPrivate::removeDefaults(name);
-	}); //queued connection, sent from within the thread to thread object on current thread
-
-	// start the thread and cache engine data
+	// create and start the engine in its thread
+	QSharedPointer<EngineThread> thread {
+		new EngineThread{name, engine, lockFile},
+		&SetupPrivate::deleteThread
+	};
 	SetupPrivate::engines.insert(name, thread);
 	thread->startEngine();
 }
 
 bool Setup::createPassive(const QString &name, int timeout)
 {
+	QMutexLocker _(&SetupPrivate::setupMutex);
+	if(SetupPrivate::engines.contains(name))
+		throw SetupExistsException(name);
+
 	// create storage dir and defaults
 	auto storageDir = d->createStorageDir(name);
 	d->createDefaults(name, storageDir, true);
 
+	// theoretically "ready" -> save (with null engine) and unlock
+	SetupPrivate::engines.insert(name, {});
+	_.unlock();
+
 	//wait for the setup to complete initialization
 	auto defaults = DefaultsPrivate::obtainDefaults(name);
 	auto isCreated = false;
@@ -501,28 +507,30 @@ bool Setup::createPassive(const QString &name, int timeout)
 
 const QString SetupPrivate::DefaultLocalDir = QStringLiteral("./qtdatasync/default");
 QMutex SetupPrivate::setupMutex(QMutex::Recursive);
-QHash<QString, QPointer<EngineThread>> SetupPrivate::engines;
+QHash<QString, QSharedPointer<EngineThread>> SetupPrivate::engines;
 unsigned long SetupPrivate::timeout = ULONG_MAX;
 
 void SetupPrivate::cleanupHandler()
 {
 	QMutexLocker _(&setupMutex);
-	auto threads = engines.values();
+	auto threads = engines;
 	engines.clear();
 	_.unlock();
 
-	for(auto &thread : threads) {
-		if(thread->stopEngine() &&
-		   thread->thread() != QThread::currentThread()) {
-			qCCritical(qdssetup) << "Stopping datasync thread" << thread->name()
-								 << "from the main thread even though it was created on a different thread!"
-								 << "This can lead to synchronization errors. You should always stop all other threads before quitting the application.";
-		}
+	// stop/remove all running setups
+	for(auto it = threads.constBegin(); it != threads.constEnd(); it++) {
+		const auto &thread = it.value();
+		if(thread) //active setup -> send stip
+			thread->stopEngine();
+		else //passive setup -> simply remove defaults
+			DefaultsPrivate::removeDefaults(it.key());
 	}
-	for(auto &thread : threads)
-		thread->waitAndTerminate(timeout);
 
-	DefaultsPrivate::clearDefaults();
+	// wait for active setups to finish
+	for(auto &thread : threads) {
+		if(thread)
+			thread->waitAndTerminate(timeout);
+	}
 }
 
 unsigned long SetupPrivate::currentTimeout()
@@ -595,9 +603,14 @@ SetupPrivate::SetupPrivate() :
 		{Defaults::SignScheme, Setup::ECDSA_ECP_SHA3_512},
 		{Defaults::CryptScheme, Setup::ECIES_ECP_SHA3_512},
 		{Defaults::SymScheme, Setup::AES_EAX}
-	}
+		}
 {}
 
+void SetupPrivate::deleteThread(EngineThread *thread)
+{
+	thread->deleteLater();
+}
+
 // ------------- Exceptions -------------
 
 SetupException::SetupException(const QString &setupName, const QString &message) :

src/datasync/setup_p.h

@@ -3,7 +3,7 @@
 
 #include <QtCore/QMutex>
 #include <QtCore/QThread>
-#include <QtCore/QPointer>
+#include <QtCore/QSharedPointer>
 
 #include <QtJsonSerializer/QJsonSerializer>
 
@@ -18,7 +18,8 @@ namespace QtDataSync {
 //must be exported for tests
 class Q_DATASYNC_EXPORT SetupPrivate
 {
-	friend class Setup;
+	friend class QtDataSync::Setup;
+	friend class QtDataSync::EngineThread;
 
 public:
 	static void cleanupHandler();
@@ -35,7 +36,7 @@ class Q_DATASYNC_EXPORT SetupPrivate
 	static const QString DefaultLocalDir;
 
 	static QMutex setupMutex;
-	static QHash<QString, QPointer<EngineThread>> engines;
+	static QHash<QString, QSharedPointer<EngineThread>> engines;
 	static unsigned long timeout;
 
 	QString localDir;
@@ -47,6 +48,8 @@ class Q_DATASYNC_EXPORT SetupPrivate
 	ExchangeEngine::ImportData initialImport;
 
 	SetupPrivate();
+
+	static void deleteThread(EngineThread *thread);
 };
 
 }