Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VFS crash and false conflict on local new. #3449

Merged
merged 5 commits into from
Jul 1, 2021
Merged

Fix VFS crash and false conflict on local new. #3449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48aefc1
Fix VFS crash and false conflict on local new.
allexzander Jun 16, 2021
c598214
Refactoring.
allexzander Jun 17, 2021
22eba41
Do not process non-directories and non-virtual files. Wipe empty onli…
allexzander Jun 17, 2021
6ce5279
Fix review comments.
allexzander Jun 18, 2021
99f6e82
Fix review comments.
allexzander Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/common/result.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ class Result
ASSERT(_isError);
return std::move(_error);
}

bool isValid() const { return !_isError; }
};

namespace detail {
Expand Down
92 changes: 57 additions & 35 deletions src/libsync/discovery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,46 +896,68 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
auto postProcessLocalNew = [item, localEntry, path, this]() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to understand why we need such complexity here when owncloud client is much simple https://github.com/owncloud/client/blob/master/src/libsync/discovery.cpp#L837*
I am under the impression that this shows that the fixes are being done at the wrong place

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after all that code was added to perform a last chance clean up of placeholder files that should not be present and we are now doing a lot of stuff needing comment to be understood
we should simplify or we will end up with code that we can no longer understand and maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to understand why we need such complexity here when owncloud client is much simple https://github.com/owncloud/client/blob/master/src/libsync/discovery.cpp#L837*
I am under the impression that this shows that the fixes are being done at the wrong place

@mgallien I am pretty sure, OwnCloud would have the same problem happening. It is just, nobody complaint about simultaneous renaming/moving of virtual files/folders.

// TODO: We may want to execute the same logic for non-VFS mode, as, moving/renaming the same folder by 2 or more clients at the same time is not possible in Web UI.
// Keeping it like this (for VFS files and folders only) just to fix a user issue.
const auto isVfsEnabled = _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() != Vfs::Off;
if (localEntry.isVirtualFile || (localEntry.isDirectory && isVfsEnabled)) {
// must be a dehydrated placeholder
const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local);

// a folder must be online-only (no files should be hydrated)
const bool isFolderPlaceholder = localEntry.isDirectory && *_discoveryData->_syncOptions._vfs->availability(path._local) == VfsItemAvailability::OnlineOnly;
if (!(_discoveryData && _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() != Vfs::Off)) {
// for VFS files and folders only
return;
}

Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW);
if (item->_instruction != CSYNC_INSTRUCTION_NEW) {
qCWarning(lcDisco) << "Wiping virtual file without db entry for" << path._local << ". But, item->_instruction is" << item->_instruction;
}
if (!localEntry.isVirtualFile && !localEntry.isDirectory) {
return;
}

// must be a file placeholder or an online-only folder placeholder
if (isFilePlaceHolder || isFolderPlaceholder) {
if (isFolderPlaceholder) {
qCInfo(lcDisco) << "Wiping virtual folder without db entry for" << path._local;
} else {
qCInfo(lcDisco) << "Wiping virtual file without db entry for" << path._local;
}
item->_instruction = CSYNC_INSTRUCTION_REMOVE;
item->_direction = SyncFileItem::Down;
// this flag needs to be unset, otherwise a folder would get marked as new in the processSubJobs
_childModified = false;
if (isFolderPlaceholder && _discoveryData) {
emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local);
}
} else {
if (localEntry.isDirectory && !isFolderPlaceholder) {
qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload.";
if (_discoveryData) {
emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local);
}
return;
}
qCWarning(lcDisco) << "Virtual file without db entry for" << path._local
<< "but looks odd, keeping";
item->_instruction = CSYNC_INSTRUCTION_IGNORE;
Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW);
if (item->_instruction != CSYNC_INSTRUCTION_NEW) {
qCWarning(lcDisco) << "Trying to wipe a virtual item" << path._local << " with item->_instruction" << item->_instruction;
return;
}

// must be a dehydrated placeholder
const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local);

// either correct availability, or a result with error if the folder is new or otherwise has no availability set yet
const auto folderPlaceHolderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityResult(Vfs::AvailabilityError::NoSuchItem);

const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : Optional<PinStateEnums::PinState>(PinState::Unspecified);

if (!isFilePlaceHolder && !folderPlaceHolderAvailability.isValid() && !folderPinState.isValid()) {
// not a file placeholder and not a synced folder placeholder (new local folder)
return;
}

const auto isFolderPinStateOnlineOnly = (folderPinState.isValid() && *folderPinState == PinState::OnlineOnly);

const auto isfolderPlaceHolderAvailabilityOnlineOnly = (folderPlaceHolderAvailability.isValid() && *folderPlaceHolderAvailability == VfsItemAvailability::OnlineOnly);

// a folder is considered online-only if: no files are hydrated, or, if it's an empty folder
const auto isOnlineOnlyFolder = isfolderPlaceHolderAvailabilityOnlineOnly || !folderPlaceHolderAvailability && isFolderPinStateOnlineOnly;

if (!isFilePlaceHolder && !isOnlineOnlyFolder) {
if (localEntry.isDirectory && folderPlaceHolderAvailability.isValid() && !isOnlineOnlyFolder) {
// a VFS folder but is not online0only (has some files hydrated)
qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload.";
emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local);
return;
}
qCWarning(lcDisco) << "Virtual file without db entry for" << path._local
<< "but looks odd, keeping";
item->_instruction = CSYNC_INSTRUCTION_IGNORE;

return;
}

if (isOnlineOnlyFolder) {
// if we're wiping a folder, we will only get this function called once and will wipe a folder along with it's files and also display one error in GUI
qCInfo(lcDisco) << "Wiping virtual folder without db entry for" << path._local;
emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local);
} else {
qCInfo(lcDisco) << "Wiping virtual file without db entry for" << path._local;
emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a file. It's going to get removed!"), path._local);
}
item->_instruction = CSYNC_INSTRUCTION_REMOVE;
item->_direction = SyncFileItem::Down;
// this flag needs to be unset, otherwise a folder would get marked as new in the processSubJobs
_childModified = false;
};

// Check if it is a move
Expand Down