1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Try harder to clean up the file info gatherer without blocking

Browse Source 
QFileSystemModel uses a QFileInfoGatherer, which is a QThread, to
populate itself with the data from the file system. Some of the system
calls we make for that might block, e.g. on Windows a disconnected
network can easily result in 10 minutes of timeout.

Destroying the QFileSystemModel destroyed the QFileInfoGatherer class
member instance, and the QFileInfoGatherer destructor, being a QThread,
had to wait on itself to avoid QThread asserting when being destroyed
while still running. This means that shutting down a Qt application that
was using a QFileSystemModel, e.g. through the non-native QFileDialog,
would hang practically indefinitely instead of shutting down.

To fix this, explicitly manage the life-time of the QFileInfoGatherer.
Manage it in QFileSystemModel through a std::unique_ptr instead of as a
direct class member so that we can control its destruction. In the
QFileSystemModel destructor, signal the gather to terminate and wait
one it for one second. If that fails, schedule the gatherer for later
deletion through deleteLater(), and release it from the unique_ptr.

The QFileInfoGatherer instance might then be alive with the
QFileSystemModel destroyed. When it receives the DeferredDelete event,
try again to abort the process and wait on the thread, this time for
5 seconds.

If that also fails, then we have to make a choice. We cannot destroy
the QThread object while it's still running, so we have to either
wait indefinitely, terminate the thread, or accept a memory leak by
ignoring the deferred delete event.

If the QCoreApplication is shutting down, then we terminate the thread.
To make this safe, explicitly control when termination is allowed in the
thread - disallow it while the thread is holding a mutex, allow it while
the thread is performing I/O operations.

If the QCoreApplication is not shutting down, then we connect to the
QThread::finished signal, and delete the object in response to that.
This might or might not happen, in which case we have to accept that
there's a memory leak.

Pick-to: 6.6
Fixes: QTBUG-120062
Change-Id: I1e4243265879d61cf406b4eeb0b11b9d59b006ad
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 0786c17bbc556a0f494fc825af47c604a9dc0241)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
Volker Hilsheimer 2023-12-14 11:31:40 +01:00 committed by Qt Cherry-pick Bot
parent 0cb01b87c0
commit 4c864a0e89
4 changed files with 95 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
src/gui/itemmodels/qfileinfogatherer.cpp
View File

@ -2,6 +2,7 @@
// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only
#include "qfileinfogatherer_p.h"
#include <qcoreapplication.h>
#include <qdebug.h>
#include <qdiriterator.h>
#include <private/qfileinfo_p.h>
@ -57,11 +58,40 @@ QFileInfoGatherer::QFileInfoGatherer(QObject *parent)
*/
QFileInfoGatherer::~QFileInfoGatherer()
{
abort.storeRelaxed(true);
requestAbort();
wait();
}
bool QFileInfoGatherer::event(QEvent *event)
{
if (event->type() == QEvent::DeferredDelete && isRunning()) {
// We have been asked to shut down later but were blocked,
// so the owning QFileSystemModel proceeded with its shut-down
// and deferred the destruction of the gatherer.
// If we are still blocked now, then we have three bad options:
// terminate, wait forever (preventing the process from shutting down),
// or accept a memory leak.
requestAbort();
if (!wait(5000)) {
// If the application is shutting down, then we terminate.
// Otherwise assume that sooner or later the thread will finish,
// and we delete it then.
if (QCoreApplication::closingDown())
terminate();
else
connect(this, &QThread::finished, this, [this]{ delete this; });
return true;
}
}
return QThread::event(event);
}
void QFileInfoGatherer::requestAbort()
{
requestInterruption();
QMutexLocker locker(&mutex);
condition.wakeAll();
locker.unlock();
wait();
}
void QFileInfoGatherer::setResolveSymlinks(bool enable)
@ -280,10 +310,13 @@ void QFileInfoGatherer::list(const QString &directoryPath)
void QFileInfoGatherer::run()
{
forever {
// Disallow termination while we are holding a mutex or can be
// woken up cleanly.
setTerminationEnabled(false);
QMutexLocker locker(&mutex);
while (!abort.loadRelaxed() && path.isEmpty())
while (!isInterruptionRequested() && path.isEmpty())
condition.wait(&mutex);
if (abort.loadRelaxed())
if (isInterruptionRequested())
return;
const QString thisPath = std::as_const(path).front();
path.pop_front();
@ -291,6 +324,10 @@ void QFileInfoGatherer::run()
files.pop_front();
locker.unlock();
// Some of the system APIs we call when gathering file infomration
// might hang (e.g. waiting for network), so we explicitly allow
// termination now.
setTerminationEnabled(true);
getFileInfos(thisPath, thisList);
}
}
@ -369,7 +406,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
QStringList allFiles;
if (files.isEmpty()) {
QDirIterator dirIt(path, QDir::AllEntries | QDir::System | QDir::Hidden);
while (!abort.loadRelaxed() && dirIt.hasNext()) {
while (!isInterruptionRequested() && dirIt.hasNext()) {
fileInfo = dirIt.nextFileInfo();
fileInfo.stat();
allFiles.append(fileInfo.fileName());
@ -380,7 +417,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
emit newListOfFiles(path, allFiles);
QStringList::const_iterator filesIt = filesToCheck.constBegin();
while (!abort.loadRelaxed() && filesIt != filesToCheck.constEnd()) {
while (!isInterruptionRequested() && filesIt != filesToCheck.constEnd()) {
fileInfo.setFile(path + QDir::separator() + *filesIt);
++filesIt;
fileInfo.stat();

6
src/gui/itemmodels/qfileinfogatherer_p.h
View File

@ -149,6 +149,8 @@ public:
QAbstractFileIconProvider *iconProvider() const;
bool resolveSymlinks() const;
void requestAbort();
public Q_SLOTS:
void list(const QString &directoryPath);
void fetchExtendedInformation(const QString &path, const QStringList &files);
@ -160,6 +162,9 @@ private Q_SLOTS:
void driveAdded();
void driveRemoved();
protected:
bool event(QEvent *event) override;
private:
void run() override;
// called by run():
@ -176,7 +181,6 @@ private:
QStack<QString> path;
QStack<QStringList> files;
// end protected by mutex
QAtomicInt abort;
#if QT_CONFIG(filesystemwatcher)
QFileSystemWatcher *m_watcher = nullptr;

73
src/gui/itemmodels/qfilesystemmodel.cpp
View File

@ -438,7 +438,7 @@ QFileSystemModelPrivate::QFileSystemNode *QFileSystemModelPrivate::node(const QS
QFileSystemModelPrivate *p = const_cast<QFileSystemModelPrivate*>(this);
node = p->addNode(parent, element,info);
#if QT_CONFIG(filesystemwatcher)
node->populate(fileInfoGatherer.getInfo(info));
node->populate(fileInfoGatherer->getInfo(info));
#endif
} else {
node = parent->children.value(element);
@ -479,7 +479,7 @@ void QFileSystemModel::timerEvent(QTimerEvent *event)
for (int i = 0; i < d->toFetch.size(); ++i) {
const QFileSystemModelPrivate::QFileSystemNode *node = d->toFetch.at(i).node;
if (!node->hasInformation()) {
d->fileInfoGatherer.fetchExtendedInformation(d->toFetch.at(i).dir,
d->fileInfoGatherer->fetchExtendedInformation(d->toFetch.at(i).dir,
QStringList(d->toFetch.at(i).file));
} else {
// qDebug("yah!, you saved a little gerbil soul");
@ -650,7 +650,7 @@ void QFileSystemModel::fetchMore(const QModelIndex &parent)
return;
indexNode->populatedChildren = true;
#if QT_CONFIG(filesystemwatcher)
d->fileInfoGatherer.list(filePath(parent));
d->fileInfoGatherer->list(filePath(parent));
#endif
}
@ -693,7 +693,7 @@ QVariant QFileSystemModel::myComputer(int role) const
return QFileSystemModelPrivate::myComputer();
#if QT_CONFIG(filesystemwatcher)
case Qt::DecorationRole:
return d->fileInfoGatherer.iconProvider()->icon(QAbstractFileIconProvider::Computer);
return d->fileInfoGatherer->iconProvider()->icon(QAbstractFileIconProvider::Computer);
#endif
}
return QVariant();
@ -734,9 +734,9 @@ QVariant QFileSystemModel::data(const QModelIndex &index, int role) const
#if QT_CONFIG(filesystemwatcher)
if (icon.isNull()) {
if (d->node(index)->isDir())
icon = d->fileInfoGatherer.iconProvider()->icon(QAbstractFileIconProvider::Folder);
icon = d->fileInfoGatherer->iconProvider()->icon(QAbstractFileIconProvider::Folder);
else
icon = d->fileInfoGatherer.iconProvider()->icon(QAbstractFileIconProvider::File);
icon = d->fileInfoGatherer->iconProvider()->icon(QAbstractFileIconProvider::File);
}
#endif // filesystemwatcher
return icon;
@ -816,7 +816,7 @@ QString QFileSystemModelPrivate::name(const QModelIndex &index) const
QFileSystemNode *dirNode = node(index);
if (
#if QT_CONFIG(filesystemwatcher)
fileInfoGatherer.resolveSymlinks() &&
fileInfoGatherer->resolveSymlinks() &&
#endif
!resolvedSymLinks.isEmpty() && dirNode->isSymLink(/* ignoreNtfsSymLinks = */ true)) {
QString fullPath = QDir::fromNativeSeparators(filePath(index));
@ -901,7 +901,7 @@ bool QFileSystemModel::setData(const QModelIndex &idx, const QVariant &value, in
nodeToRename->fileName = newName;
nodeToRename->parent = parentNode;
#if QT_CONFIG(filesystemwatcher)
nodeToRename->populate(d->fileInfoGatherer.getInfo(QFileInfo(parentPath, newName)));
nodeToRename->populate(d->fileInfoGatherer->getInfo(QFileInfo(parentPath, newName)));
#endif
nodeToRename->isVisible = true;
parentNode->children[newName] = nodeToRename.release();
@ -1326,7 +1326,7 @@ void QFileSystemModel::setOptions(Options options)
#if QT_CONFIG(filesystemwatcher)
Q_D(QFileSystemModel);
if (changed.testFlag(DontWatchForChanges))
d->fileInfoGatherer.setWatching(!options.testFlag(DontWatchForChanges));
d->fileInfoGatherer->setWatching(!options.testFlag(DontWatchForChanges));
#endif
if (changed.testFlag(DontUseCustomDirectoryIcons)) {
@ -1347,7 +1347,7 @@ QFileSystemModel::Options QFileSystemModel::options() const
result.setFlag(DontResolveSymlinks, !resolveSymlinks());
#if QT_CONFIG(filesystemwatcher)
Q_D(const QFileSystemModel);
result.setFlag(DontWatchForChanges, !d->fileInfoGatherer.isWatching());
result.setFlag(DontWatchForChanges, !d->fileInfoGatherer->isWatching());
#else
result.setFlag(DontWatchForChanges);
#endif
@ -1369,7 +1369,7 @@ QString QFileSystemModel::filePath(const QModelIndex &index) const
QFileSystemModelPrivate::QFileSystemNode *dirNode = d->node(index);
if (dirNode->isSymLink()
#if QT_CONFIG(filesystemwatcher)
&& d->fileInfoGatherer.resolveSymlinks()
&& d->fileInfoGatherer->resolveSymlinks()
#endif
&& d->resolvedSymLinks.contains(fullPath)
&& dirNode->isDir()) {
@ -1431,7 +1431,7 @@ QModelIndex QFileSystemModel::mkdir(const QModelIndex &parent, const QString &na
Q_ASSERT(parentNode->children.contains(name));
QFileSystemModelPrivate::QFileSystemNode *node = parentNode->children[name];
#if QT_CONFIG(filesystemwatcher)
node->populate(d->fileInfoGatherer.getInfo(QFileInfo(dir.absolutePath() + QDir::separator() + name)));
node->populate(d->fileInfoGatherer->getInfo(QFileInfo(dir.absolutePath() + QDir::separator() + name)));
#endif
d->addVisibleFiles(parentNode, QStringList(name));
return d->index(node);
@ -1499,7 +1499,7 @@ QModelIndex QFileSystemModel::setRootPath(const QString &newPath)
if (!rootPath().isEmpty() && rootPath() != "."_L1) {
//This remove the watcher for the old rootPath
#if QT_CONFIG(filesystemwatcher)
d->fileInfoGatherer.removePath(rootPath());
d->fileInfoGatherer->removePath(rootPath());
#endif
//This line "marks" the node as dirty, so the next fetchMore
//call on the path will ask the gatherer to install a watcher again
@ -1555,7 +1555,7 @@ void QFileSystemModel::setIconProvider(QAbstractFileIconProvider *provider)
{
Q_D(QFileSystemModel);
#if QT_CONFIG(filesystemwatcher)
d->fileInfoGatherer.setIconProvider(provider);
d->fileInfoGatherer->setIconProvider(provider);
#endif
d->root.updateIcon(provider, QString());
}
@ -1567,7 +1567,7 @@ QAbstractFileIconProvider *QFileSystemModel::iconProvider() const
{
#if QT_CONFIG(filesystemwatcher)
Q_D(const QFileSystemModel);
return d->fileInfoGatherer.iconProvider();
return d->fileInfoGatherer->iconProvider();
#else
return 0;
#endif
@ -1623,7 +1623,7 @@ void QFileSystemModel::setResolveSymlinks(bool enable)
{
#if QT_CONFIG(filesystemwatcher)
Q_D(QFileSystemModel);
d->fileInfoGatherer.setResolveSymlinks(enable);
d->fileInfoGatherer->setResolveSymlinks(enable);
#else
Q_UNUSED(enable);
#endif
@ -1633,7 +1633,7 @@ bool QFileSystemModel::resolveSymlinks() const
{
#if QT_CONFIG(filesystemwatcher)
Q_D(const QFileSystemModel);
return d->fileInfoGatherer.resolveSymlinks();
return d->fileInfoGatherer->resolveSymlinks();
#else
return false;
#endif
@ -1738,7 +1738,7 @@ bool QFileSystemModel::event(QEvent *event)
#if QT_CONFIG(filesystemwatcher)
Q_D(QFileSystemModel);
if (event->type() == QEvent::LanguageChange) {
d->root.retranslateStrings(d->fileInfoGatherer.iconProvider(), QString());
d->root.retranslateStrings(d->fileInfoGatherer->iconProvider(), QString());
return true;
}
#endif
@ -1752,7 +1752,7 @@ bool QFileSystemModel::rmdir(const QModelIndex &aindex)
#if QT_CONFIG(filesystemwatcher)
if (success) {
QFileSystemModelPrivate * d = const_cast<QFileSystemModelPrivate*>(d_func());
d->fileInfoGatherer.removePath(path);
d->fileInfoGatherer->removePath(path);
}
#endif
return success;
@ -1924,7 +1924,7 @@ void QFileSystemModelPrivate::_q_fileSystemChanged(const QString &path,
for (const auto &update : updates) {
QString fileName = update.first;
Q_ASSERT(!fileName.isEmpty());
QExtendedInformation info = fileInfoGatherer.getInfo(update.second);
QExtendedInformation info = fileInfoGatherer->getInfo(update.second);
bool previouslyHere = parentNode->children.contains(fileName);
if (!previouslyHere) {
addNode(parentNode, fileName, info.fileInfo());
@ -2055,22 +2055,37 @@ QStringList QFileSystemModelPrivate::unwatchPathsAt(const QModelIndex &index)
return false;
};
const QStringList &watchedFiles = fileInfoGatherer.watchedFiles();
const QStringList &watchedFiles = fileInfoGatherer->watchedFiles();
std::copy_if(watchedFiles.cbegin(), watchedFiles.cend(),
std::back_inserter(result), filter);
const QStringList &watchedDirectories = fileInfoGatherer.watchedDirectories();
const QStringList &watchedDirectories = fileInfoGatherer->watchedDirectories();
std::copy_if(watchedDirectories.cbegin(), watchedDirectories.cend(),
std::back_inserter(result), filter);
fileInfoGatherer.unwatchPaths(result);
fileInfoGatherer->unwatchPaths(result);
return result;
}
#endif // filesystemwatcher && Q_OS_WIN
QFileSystemModelPrivate::QFileSystemModelPrivate() = default;
QFileSystemModelPrivate::QFileSystemModelPrivate()
: fileInfoGatherer(new QFileInfoGatherer)
{
}
QFileSystemModelPrivate::~QFileSystemModelPrivate() = default;
QFileSystemModelPrivate::~QFileSystemModelPrivate()
{
fileInfoGatherer->requestInterruption();
if (!fileInfoGatherer->wait(1000)) {
// If the thread hangs, perhaps because the network was disconnected
// while the gatherer was stat'ing a remote file, then don't block
// shutting down the model (which might block a file dialog and the
// main thread). Schedule the gatherer for later deletion; it's
// destructor will wait for the thread to finish.
auto *rawGatherer = fileInfoGatherer.release();
rawGatherer->deleteLater();
}
}
/*!
\internal
@ -2083,13 +2098,13 @@ void QFileSystemModelPrivate::init()
qRegisterMetaType<QList<std::pair<QString, QFileInfo>>>();
#if QT_CONFIG(filesystemwatcher)
q->connect(&fileInfoGatherer, SIGNAL(newListOfFiles(QString,QStringList)),
q->connect(fileInfoGatherer.get(), SIGNAL(newListOfFiles(QString,QStringList)),
q, SLOT(_q_directoryChanged(QString,QStringList)));
q->connect(&fileInfoGatherer, SIGNAL(updates(QString,QList<std::pair<QString,QFileInfo>>)), q,
q->connect(fileInfoGatherer.get(), SIGNAL(updates(QString,QList<std::pair<QString,QFileInfo>>)), q,
SLOT(_q_fileSystemChanged(QString,QList<std::pair<QString,QFileInfo>>)));
q->connect(&fileInfoGatherer, SIGNAL(nameResolved(QString,QString)),
q->connect(fileInfoGatherer.get(), SIGNAL(nameResolved(QString,QString)),
q, SLOT(_q_resolvedName(QString,QString)));
q->connect(&fileInfoGatherer, SIGNAL(directoryLoaded(QString)),
q->connect(fileInfoGatherer.get(), SIGNAL(directoryLoaded(QString)),
q, SIGNAL(directoryLoaded(QString)));
#endif // filesystemwatcher
q->connect(&delayedSortTimer, SIGNAL(timeout()), q, SLOT(_q_performDelayedSort()), Qt::QueuedConnection);

4
src/gui/itemmodels/qfilesystemmodel_p.h
View File

@ -259,9 +259,9 @@ public:
#if QT_CONFIG(filesystemwatcher)
# ifdef Q_OS_WIN
QStringList unwatchPathsAt(const QModelIndex &);
void watchPaths(const QStringList &paths) { fileInfoGatherer.watchPaths(paths); }
void watchPaths(const QStringList &paths) { fileInfoGatherer->watchPaths(paths); }
# endif // Q_OS_WIN
QFileInfoGatherer fileInfoGatherer;
std::unique_ptr<QFileInfoGatherer> fileInfoGatherer;
#endif // filesystemwatcher
QTimer delayedSortTimer;
QHash<const QFileSystemNode*, bool> bypassFilters;