Commit 24b0edd1 authored by Michael Pyne's avatar Michael Pyne

Fix crash with threaded file loading.

Although Qt protects access to functions in the GUI thread (where I was
seeing crashes in the new threaded file loading code in some
situations), as long as you use signals/slots, Qt Concurrent doesn't do
anything to keep your non-GUI threaded code from trampling on each
other.

Which is obvious enough, in retrospect, but that seems to have been the
reason for the crashes I was sometimes seeing (TagLib and/or FileHandle
not being thread-safe).

The immediate bugfix is to serialize access into FileHandle/TagLib file
reading code in DirectoryLoader, though I also did some cleanups in the
process of debugging that I think are worth keeping.
parent 0930c648
Pipeline #1038 passed with stage
in 9 minutes and 5 seconds
......@@ -17,6 +17,8 @@
#include "directoryloader.h"
#include <QFileInfo>
#include <QMutex>
#include <QMutexLocker>
#include "mediafiles.h"
......@@ -81,8 +83,6 @@ void DirectoryLoader::startLoading()
if(!files.isEmpty()) {
emit loadedFiles(files);
}
emit doneLoading();
}
MediaFileType classifyFile(const QFileInfo &fileInfo)
......@@ -111,6 +111,13 @@ MediaFileType classifyFile(const QFileInfo &fileInfo)
FileHandle loadMediaFile(const QString &fileName)
{
// Just because our GUI thread accesses are serialized by signal/slot magic
// doesn't mean that our non-GUI threads don't require deconfliction
// Neither FileHandle nor TagLib are not thread-safe so synchronize access
// to this function.
static QMutex fhLock;
QMutexLocker locker(&fhLock);
FileHandle loadedMetadata(fileName);
(void) loadedMetadata.tag(); // Ensure tag is read
......
......@@ -37,7 +37,6 @@ public slots:
void startLoading();
signals:
void doneLoading();
void loadedFiles(FileHandleList files);
void loadedPlaylist(QString fileName);
......
......@@ -56,6 +56,7 @@
#include <QPainter>
#include <QtConcurrent>
#include <QFutureWatcher>
#include <id3v1genres.h>
......@@ -1028,23 +1029,44 @@ void Playlist::createItems(const PlaylistItemList &siblings, PlaylistItem *after
void Playlist::addFiles(const QStringList &files, PlaylistItem *after)
{
m_blockDataChanged = true;
m_itemsLoading++;
if(Q_UNLIKELY(files.isEmpty())) {
return;
}
m_blockDataChanged = true;
setEnabled(false);
QVector<QFuture<void>> pendingFutures;
for(const auto &file : files) {
// some files added here will launch threads that will do cleanup (fix
// the cursor, allow data updates etc) when the last thread is done.
// Managed by m_itemsLoading going to 0 which is why we ++ above.
addUntypedFile(file, after);
// some files added here will launch threads that we must wait until
// they're done to cleanup
auto pendingResult = addUntypedFile(file, after);
if(!pendingResult.isFinished()) {
pendingFutures.push_back(pendingResult);
++m_itemsLoading;
}
}
// If no items are being loaded by now then we must have loaded all M3U
// playlists or something, so cleanup immediately since no threads will
// have been launched.
if(--m_itemsLoading == 0) {
// It's possible for no async threads to be launched, and also possible
// for this function to be called while there were other threads in flight
if(pendingFutures.isEmpty() && m_itemsLoading == 0) {
cleanupAfterAllFileLoadsCompleted();
return;
}
// Build handlers for all the still-active loaders on the heap and then
// return to the event loop.
for(const auto &future : qAsConst(pendingFutures)) {
auto loadWatcher = new QFutureWatcher<void>(this);
loadWatcher->setFuture(future);
connect(loadWatcher, &QFutureWatcher<void>::finished, this, [=]() {
if(--m_itemsLoading == 0) {
cleanupAfterAllFileLoadsCompleted();
}
loadWatcher->deleteLater();
});
}
}
......@@ -1521,20 +1543,9 @@ void Playlist::addPlaylistFile(const QString &m3uFile)
}
}
void Playlist::addFilesFromDirectory(const QString &dirPath)
QFuture<void> Playlist::addFilesFromDirectory(const QString &dirPath)
{
++m_itemsLoading;
DirectoryLoader *loader = new DirectoryLoader(dirPath);
connect(loader, &DirectoryLoader::doneLoading, this,
[this, loader]() {
loader->deleteLater();
if(--m_itemsLoading == 0) {
cleanupAfterAllFileLoadsCompleted();
}
}
);
auto loader = new DirectoryLoader(dirPath);
connect(loader, &DirectoryLoader::loadedPlaylist, this,
[this](const QString &m3uFile) {
......@@ -1549,13 +1560,21 @@ void Playlist::addFilesFromDirectory(const QString &dirPath)
}
);
(void) QtConcurrent::run(loader, &DirectoryLoader::startLoading);
auto future = QtConcurrent::run(loader, &DirectoryLoader::startLoading);
auto loadWatcher = new QFutureWatcher<void>(this);
connect(loadWatcher, &QFutureWatcher<void>::finished, this, [=]() {
loader->deleteLater();
loadWatcher->deleteLater();
});
return future;
}
void Playlist::addUntypedFile(const QString &file, PlaylistItem *after)
// Returns a future since some codepaths will result in an async operation.
QFuture<void> Playlist::addUntypedFile(const QString &file, PlaylistItem *after)
{
if(hasItem(file) && !m_allowDuplicates)
return;
return {};
const QFileInfo fileInfo(file);
const QString canonicalPath = fileInfo.canonicalFilePath();
......@@ -1566,22 +1585,24 @@ void Playlist::addUntypedFile(const QString &file, PlaylistItem *after)
FileHandle f(fileInfo);
f.tag();
createItem(f, after);
return;
return {};
}
if(MediaFiles::isPlaylistFile(file)) {
addPlaylistFile(canonicalPath);
return;
return {};
}
if(fileInfo.isDir()) {
foreach(const QString &directory, m_collection->excludedFolders()) {
if(canonicalPath.startsWith(directory))
return; // Exclude it
return {}; // Exclude it
}
addFilesFromDirectory(canonicalPath);
return addFilesFromDirectory(canonicalPath);
}
return {};
}
// Called directly or after a threaded directory load has completed, managed by
......
......@@ -24,6 +24,7 @@
#include <QEvent>
#include <QList>
#include <QTreeWidget>
#include <QFuture>
#include "covermanager.h"
#include "stringhash.h"
......@@ -545,8 +546,8 @@ private:
void calculateColumnWeights();
void addPlaylistFile(const QString &m3uFile);
void addFilesFromDirectory(const QString &dirPath);
void addUntypedFile(const QString &file, PlaylistItem *after = nullptr);
QFuture<void> addFilesFromDirectory(const QString &dirPath);
QFuture<void> addUntypedFile(const QString &file, PlaylistItem *after = nullptr);
void cleanupAfterAllFileLoadsCompleted();
void redisplaySearch() { setSearch(m_search); }
......
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