SMBSync3 icon indicating copy to clipboard operation
SMBSync3 copied to clipboard

Sync bugs with unexpected deletes

Open PhilZ-cwm6 opened this issue 5 years ago • 1 comments

Hi, Out of curiosity I looked quickly at the sync code in your current version. I know it is beta, so I never looked at it before, but I see that you largely edited the sync checks to delete files and they are again broken like before

https://github.com/Sentaroh/SMBSync3/blob/main/app/src/main/java/com/sentaroh/android/SMBSync3/SyncThreadSyncFile.java#L167

  • many useless overhead with isDirectoryToBeProcessed() and mf.exists() calls on each operation while not needed always
  • worst, the checks here: https://github.com/Sentaroh/SMBSync3/blob/main/app/src/main/java/com/sentaroh/android/SMBSync3/SyncThreadSyncFile.java#L201 They are wrong and will delete dirs always if !mf.exists() + don't always respect process hidden dir option
  • file deletes: the code seems ok, but the overhead with mf.exists() && isFileSelected() is considerable. Also, on an smb server the mf.exists() is certainly considerably slower than isFileSelected() or isDirectoryToBeProcessed()

I did not look at the filters checks if code was changed for isFileSelected() and is DirectoryToBeProcessed() but the implementation was also optimized for overhead. The only project I know of and that has a non broken code is the one I linked at (wildcard-project). However, they use a char by char check instead of ever calling a pattern check. Speed is same but more prone to custom code bugs. Hope you deeply verify and check all possibilities if you modify it.

Note: I am taking a long break and do not plan to push commits except fr/en translations from time to time. I plan to migrate to Android R by end of year or begin 2021, so I am not migrating before that to SMBSync3 and won't really look at it before.

I will push the xda and Phone Android threads since the fixed filters v2 is now enabled.

Thank you for your free time on the project and Best regards

PhilZ-cwm6 avatar Jul 28 '20 13:07 PhilZ-cwm6

I took a very quick look in one function SyncThreadSyncFile.java/LocalToLocal() New code is much better and looks ok in regard of unexpected deletes/non deletes

There is still a room for a potential big improvement in sync speed when filter excludes many files or folders by wildcards for example (exclude *.jpg, *.tmp...). Your current code will check each excluded file/folder if it exists on master. The operation is very time consuming depending on the file system, and worst if master is SMB.

I suggest you this implementation, maybe more clear than the one in my original code in SMBSync2 (I only put the directory part, but file part is same logic).

Your current code for directories:

boolean mf_exists=mf.exists();
boolean isDirectoryToBeProcessed=SyncThread.isDirectoryToBeProcessed(stwa, relative_dir);
if (mf_exists && isDirectoryToBeProcessed) {
    //process subdirs
} else {
    if ((!mf_exists && isDirectoryToBeProcessed) ||
            (sti.isSyncOptionRemoveDirectoryFileThatExcludedByFilter() && !isDirectoryToBeProcessed)) {
        //delete target item
    }
}

Change it to this logic:

boolean isDirectoryToBeProcessed=SyncThread.isDirectoryToBeProcessed(stwa, relative_dir);

if (isDirectoryToBeProcessed) {
    if (mf.exists()) {
        //process subdirs
    } else {
        //delete target item
    }
} else { //excluded by filters, never check if file/dir exists on master
    if (sti.isSyncOptionRemoveDirectoryFileThatExcludedByFilter()) {
        //delete target item
    } else {
        //nothing to do, skip
    }
}

The code is more clear and if a file/dir is excluded by filters, you never check if it exists on the master. The impact can be huge as explained above when there is a filter that excludes many files/dirs by generic wildcards (*.tmp, *.jpg, ...)

Again, I am not planning to compile/test it for now, but I am interested in all these future developments.

Best regards

PhilZ-cwm6 avatar Aug 20 '20 23:08 PhilZ-cwm6