diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp index ab99460a0f2a6..603d46ee06196 100644 --- a/src/common/syncjournaldb.cpp +++ b/src/common/syncjournaldb.cpp @@ -703,6 +703,7 @@ void SyncJournalDb::close() _db.close(); clearEtagStorageFilter(); + _selectiveSyncListCache.clear(); _metadataTableIsEmpty = false; } @@ -2380,6 +2381,12 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList return result; } + const auto cached = _selectiveSyncListCache.constFind(int(type)); + if (cached != _selectiveSyncListCache.constEnd()) { + *ok = true; + return *cached; + } + const auto query = _queryManager.get(PreparedSqlQueryManager::GetSelectiveSyncListQuery, QByteArrayLiteral("SELECT path FROM selectivesync WHERE type=?1"), _db); if (!query) { qCWarning(lcDb) << "database error:" << query->error(); @@ -2408,6 +2415,8 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList } *ok = true; + _selectiveSyncListCache.insert(int(type), result); + return result; } @@ -2418,6 +2427,10 @@ void SyncJournalDb::setSelectiveSyncList(SyncJournalDb::SelectiveSyncListType ty return; } + // Dropped rather than replaced with `list`: getSelectiveSyncList() hands out entries with a + // trailing slash, which is added on read, not on write. + _selectiveSyncListCache.remove(int(type)); + startTransaction(); //first, delete all entries of this type diff --git a/src/common/syncjournaldb.h b/src/common/syncjournaldb.h index 8cd868e1f0b71..84cd10dc69a3a 100644 --- a/src/common/syncjournaldb.h +++ b/src/common/syncjournaldb.h @@ -431,6 +431,15 @@ public slots: int _transaction = 0; bool _metadataTableIsEmpty = false; + /* Cache for getSelectiveSyncList(), keyed by SelectiveSyncListType. + * + * The list is read once per file in some hot paths (see Folder::warnOnNewExcludedItem) while + * changing only when the user edits the selective sync configuration, so it is kept here + * rather than re-queried. Invalidated by setSelectiveSyncList() -- the only writer of the + * selectivesync table -- and on close(). + */ + QHash _selectiveSyncListCache; + /* Storing etags to these folders, or their parent folders, is filtered out. * * When schedulePathForRemoteDiscovery() is called some etags to _invalid_ in the diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 074c87ee42895..63ea4070f45bd 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -1203,6 +1203,10 @@ void Folder::startSync(const QStringList &pathList) fullLocalDiscoveryInterval.count() >= 0 // negative means we don't require periodic full runs && _timeSinceLastFullLocalDiscovery.hasExpired(fullLocalDiscoveryInterval.count()); + // From here on the sync's discovery style is fixed, so a request arriving later is about a + // state this sync will not see and must not be marked as covered by it. + _fullLocalDiscoveryRequestedDuringSync = false; + if (singleItemDiscoveryOptions.isValid() && singleItemDiscoveryOptions.discoveryPath != QStringLiteral("/")) { qCInfo(lcFolder) << "Going to sync just one file"; _engine->setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, {singleItemDiscoveryOptions.discoveryPath}); @@ -1369,7 +1373,8 @@ void Folder::slotSyncFinished(bool success) if ((_syncResult.status() == SyncResult::Success || _syncResult.status() == SyncResult::Problem) && success) { - if (_engine->lastLocalDiscoveryStyle() == LocalDiscoveryStyle::FilesystemOnly) { + if (_engine->lastLocalDiscoveryStyle() == LocalDiscoveryStyle::FilesystemOnly + && !_fullLocalDiscoveryRequestedDuringSync) { _timeSinceLastFullLocalDiscovery.start(); } } @@ -1588,6 +1593,10 @@ void Folder::slotScheduleThisFolder() void Folder::slotNextSyncFullLocalDiscovery() { _timeSinceLastFullLocalDiscovery.invalidate(); + + // Invalidating on its own is not enough: a sync that is already running restarts the timer when + // it finishes (see slotSyncFinished), which would drop this request. + _fullLocalDiscoveryRequestedDuringSync = true; } void Folder::setSilenceErrorsUntilNextSync(bool silenceErrors) @@ -1618,15 +1627,8 @@ void Folder::warnOnNewExcludedItem(const SyncJournalFileRecord &record, const QS return; } - // Don't warn for items that no longer exist. - // Note: This assumes we're getting file watcher notifications - // for folders only on creation and deletion - if we got a notification - // on content change that would create spurious warnings. - const auto fullPath = QString{_canonicalLocalPath + path}; - if (!FileSystem::fileExists(fullPath)) { - return; - } - + // Checked before touching the filesystem below: this runs once per added file, and the list is + // empty unless the user configured selective sync. bool ok = false; const auto selectiveSyncList = _journal.getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok); if (!ok) { @@ -1636,6 +1638,15 @@ void Folder::warnOnNewExcludedItem(const SyncJournalFileRecord &record, const QS return; } + // Don't warn for items that no longer exist. + // Note: This assumes we're getting file watcher notifications + // for folders only on creation and deletion - if we got a notification + // on content change that would create spurious warnings. + const auto fullPath = QString{_canonicalLocalPath + path}; + if (!FileSystem::fileExists(fullPath)) { + return; + } + QFileInfo excludeItemFileInfo(fullPath); const auto excludeItemFilePath = excludeItemFileInfo.filePath(); const auto message = FileSystem::isDir(fullPath) @@ -1749,6 +1760,10 @@ void Folder::registerFolderWatcher() this, [this](const QString &path) { slotWatchedPathChanged(path, Folder::ChangeReason::Other); }); connect(_folderWatcher.data(), &FolderWatcher::lostChanges, this, &Folder::slotNextSyncFullLocalDiscovery); + // The watcher reports no paths along with lostChanges(), so nothing else would trigger the + // sync that is supposed to pick the missed changes up. + connect(_folderWatcher.data(), &FolderWatcher::lostChanges, + this, &Folder::scheduleThisFolderSoon); connect(_folderWatcher.data(), &FolderWatcher::becameUnreliable, this, &Folder::slotWatcherUnreliable); if (_accountState->account()->capabilities().filesLockAvailable()) { @@ -1769,6 +1784,7 @@ void Folder::disconnectFolderWatcher() } disconnect(_folderWatcher.data(), &FolderWatcher::pathChanged, nullptr, nullptr); disconnect(_folderWatcher.data(), &FolderWatcher::lostChanges, this, &Folder::slotNextSyncFullLocalDiscovery); + disconnect(_folderWatcher.data(), &FolderWatcher::lostChanges, this, &Folder::scheduleThisFolderSoon); disconnect(_folderWatcher.data(), &FolderWatcher::becameUnreliable, this, &Folder::slotWatcherUnreliable); if (_accountState->account()->capabilities().filesLockAvailable()) { disconnect(_folderWatcher.data(), &FolderWatcher::filesLockReleased, this, &Folder::slotFilesLockReleased); diff --git a/src/gui/folder.h b/src/gui/folder.h index 94ce4abdef259..bd642c81366db 100644 --- a/src/gui/folder.h +++ b/src/gui/folder.h @@ -580,6 +580,14 @@ private slots: QElapsedTimer _timeSinceLastSyncDone; QElapsedTimer _timeSinceLastSyncStart; QElapsedTimer _timeSinceLastFullLocalDiscovery; + + /** Whether a full local discovery was requested after the running sync picked its discovery + * style, and is therefore not covered by it. + * + * Without this, slotSyncFinished() would restart _timeSinceLastFullLocalDiscovery and the + * request would be lost. + */ + bool _fullLocalDiscoveryRequestedDuringSync = false; std::chrono::milliseconds _lastSyncDuration; /// The number of syncs that failed in a row. diff --git a/src/gui/folderwatcher.cpp b/src/gui/folderwatcher.cpp index 2f522ee8fad3f..1cc2b03f71ae9 100644 --- a/src/gui/folderwatcher.cpp +++ b/src/gui/folderwatcher.cpp @@ -213,6 +213,17 @@ int FolderWatcher::lockChangeDebouncingTimout() const return _lockChangeDebouncingTimer.interval(); } +void FolderWatcher::changesLost(const QString &root) +{ + qCWarning(lcFolderWatcher) << "Lost track of individual changes in" << root + << "- the next sync will run a full local discovery"; + + // Makes the next sync rediscover the tree from the filesystem instead of the database. No path + // is reported: the root would reach the local discovery tracker as an empty relative path, + // which matches only the root itself and not the tree below it. + emit lostChanges(); +} + void FolderWatcher::changeDetected(const QString &path) { QStringList paths(path); diff --git a/src/gui/folderwatcher.h b/src/gui/folderwatcher.h index 1e13a8b29c13d..544273b236920 100644 --- a/src/gui/folderwatcher.h +++ b/src/gui/folderwatcher.h @@ -129,6 +129,14 @@ private slots: void startNotificationTestWhenReady(); void lockChangeDebouncingTimerTimedOut(); +private: + /** Report that the backend could not keep track of individual changes under @a root. + * + * Emits lostChanges() so that the next sync runs a full local discovery. Reports no path, so + * whoever wants a sync scheduled has to react to lostChanges() as well. + */ + void changesLost(const QString &root); + protected: QHash _pendingPathes; diff --git a/src/gui/folderwatcher_mac.cpp b/src/gui/folderwatcher_mac.cpp index b4105aac8d954..f6787e57ac6f1 100644 --- a/src/gui/folderwatcher_mac.cpp +++ b/src/gui/folderwatcher_mac.cpp @@ -13,11 +13,22 @@ #include #include +#include #include namespace OCC { +namespace { +/* Upper bound on the paths one event batch may be expanded into by addCoalescedPaths(). + * + * Every reported path costs a journal lookup and a few stats on the GUI thread later on, so for a + * large tree it is both faster and gentler on the UI to stop enumerating and let the next sync + * rediscover the tree instead. + */ +constexpr auto maxCoalescedPaths = 1000; +} + FolderWatcherPrivate::FolderWatcherPrivate(FolderWatcher *p, const QString &path) : _parent(p) , _folder(path) @@ -27,9 +38,20 @@ FolderWatcherPrivate::FolderWatcherPrivate(FolderWatcher *p, const QString &path FolderWatcherPrivate::~FolderWatcherPrivate() { - FSEventStreamStop(_stream); - FSEventStreamInvalidate(_stream); - FSEventStreamRelease(_stream); + if (_stream) { + FSEventStreamStop(_stream); + FSEventStreamInvalidate(_stream); + FSEventStreamRelease(_stream); + _stream = nullptr; + } + + if (_queue) { + // A callback block may already be running on the queue and still touching this object, + // so let it drain before the members it reads are destroyed. + dispatch_sync(_queue, ^{}); + dispatch_release(_queue); + _queue = nullptr; + } } static void callback( @@ -108,44 +130,71 @@ void FolderWatcherPrivate::startWatching() CFRelease(pathsToWatch); CFRelease(folderCF); - FSEventStreamSetDispatchQueue(_stream, dispatch_get_main_queue()); + + // Deliver events on a private queue rather than the main one: dropping a large tree into the + // sync folder produces event batches big enough that handling them on the main queue blocks + // the GUI. This mirrors what the Windows watcher does with its own WatcherThread. + _queue = dispatch_queue_create("com.nextcloud.desktopclient.folderwatcher", DISPATCH_QUEUE_SERIAL); + FSEventStreamSetDispatchQueue(_stream, _queue); FSEventStreamStart(_stream); } -QStringList FolderWatcherPrivate::addCoalescedPaths(const QStringList &paths) const +void FolderWatcherPrivate::addCoalescedPaths(const QStringList &paths, QStringList &coalesced) const { - QStringList coalescedPaths; + // Only the expansion below is bounded, not `paths` itself: those are events FSEvents actually + // reported, they are capped by the kernel's own buffer, and reporting them is what drives + // things a rediscovery cannot redo, such as releasing office file locks. + // + // FSEvents reports a renamed or moved directory, but not the items below it, so the tree is + // walked to report those too. + coalesced = paths; + QSet seen{paths.begin(), paths.end()}; + for (const auto &eventPath : paths) { - if (QDir(eventPath).exists()) { - QDirIterator it(eventPath, QDir::AllDirs | QDir::NoDotAndDotDot | QDir::Files, QDirIterator::Subdirectories); - while (it.hasNext()) { - const auto path = it.next(); - if (!paths.contains(path)) { - coalescedPaths.append(path); - } + if (!QDir(eventPath).exists()) { + continue; + } + QDirIterator it(eventPath, QDir::AllDirs | QDir::NoDotAndDotDot | QDir::Files, QDirIterator::Subdirectories); + while (it.hasNext()) { + const auto path = it.next(); + if (coalesced.size() >= maxCoalescedPaths) { + // Give up on expanding this batch rather than report a whole tree path by path. + // Nothing is lost by stopping: the directory that is being expanded was itself + // reported, and SyncEngine::shouldDiscoverLocally() descends into a touched + // directory's entire subtree, so the entries below it are discovered anyway. + return; + } + if (!seen.contains(path)) { + seen.insert(path); + coalesced.append(path); } } } - return (paths + coalescedPaths); } void FolderWatcherPrivate::doNotifyParent(const QStringList &paths) { - const QStringList totalPaths = addCoalescedPaths(paths); - _parent->changeDetected(totalPaths); + if (paths.isEmpty()) { + return; + } + + QStringList totalPaths; + addCoalescedPaths(paths, totalPaths); + + // We are on the private dispatch queue here, but everything FolderWatcher touches from + // changeDetected() (its timers, the lock-file sets, the pathChanged() receivers) may only be + // used on the thread it lives on, so hand the batch over instead of processing it here. + QMetaObject::invokeMethod( + _parent, [parent = _parent, totalPaths] { parent->changeDetected(totalPaths); }, Qt::QueuedConnection); } void FolderWatcherPrivate::notifyAll() { - QDirIterator dirIterator(_folder, QDirIterator::Subdirectories); - QStringList allPaths; - - while(dirIterator.hasNext()) { - const auto dirEntry = dirIterator.next(); - allPaths.append(dirEntry); - } - - _parent->changeDetected(allPaths); + // FSEvents either dropped events or asked for a full rescan. Enumerating the whole tree to + // report every path would take minutes on a large folder, and a full local discovery re-reads + // it anyway, so ask for one of those instead of reporting anything. + QMetaObject::invokeMethod( + _parent, [parent = _parent, folder = _folder] { parent->changesLost(folder); }, Qt::QueuedConnection); } diff --git a/src/gui/folderwatcher_mac.h b/src/gui/folderwatcher_mac.h index 723227bebb2d8..092ff274309ee 100644 --- a/src/gui/folderwatcher_mac.h +++ b/src/gui/folderwatcher_mac.h @@ -26,7 +26,14 @@ class FolderWatcherPrivate ~FolderWatcherPrivate(); void startWatching(); - QStringList addCoalescedPaths(const QStringList &) const; + + /** Expand @a paths with the contents of any directory among them, into @a coalesced. + * + * The expansion stops at maxCoalescedPaths; the sync engine descends into a reported directory + * on its own, so the entries left out are still discovered. @a paths itself is always passed + * through, however large it is. + */ + void addCoalescedPaths(const QStringList &paths, QStringList &coalesced) const; void doNotifyParent(const QStringList &); void notifyAll(); @@ -38,7 +45,10 @@ class FolderWatcherPrivate QString _folder; - FSEventStreamRef _stream; + FSEventStreamRef _stream = nullptr; + + /// Events are delivered here instead of on the main queue, to keep them off the GUI thread. + dispatch_queue_t _queue = nullptr; }; } diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 8c754d3eac718..cb571759413b4 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -30,6 +30,7 @@ #include #endif +#include #include #include #include @@ -473,9 +474,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) checkErrorBlacklisting(*item); _needsUpdate = true; - // Insert sorted - auto it = std::lower_bound( _syncItems.begin(), _syncItems.end(), item ); // the _syncItems is sorted - _syncItems.insert( it, item ); + // Consumers need _syncItems sorted, but inserting in order here would memmove the tail of the + // vector once per item and cost O(n^2) overall. Nothing reads _syncItems before discovery ends, + // so it is sorted in one pass in slotDiscoveryFinished() instead. + _syncItems.append(item); slotNewItem(item); @@ -847,6 +849,10 @@ void SyncEngine::slotDiscoveryFinished() _restartedSyncAfterDiscovery = false; } + // Restores the ordering slotItemDiscovered() deliberately did not maintain per item. Stable so + // that items comparing equal keep the order they were discovered in. + std::stable_sort(_syncItems.begin(), _syncItems.end()); + if (handleMassDeletion()) { return; }