scoped storage fixes and improvements

[Xavron]

Highly recommended to apply this pull request.

Closes:

• Minor invalid name or chroot violation issue #200

and major code improvement for DocumentFile.

Short version:

Heavily simplified better code. Can chew through 7,000 files in one dir using DocumentFile much faster than before and "a/a a" delete in WinSCP is no longer a problem as reworking the code was needed to fix it properly.

Long version:

The looped code found on stackoverflow caused too many problems so this gets rid of that completely. Apparently, it wasn't needed anyway. Looks like someone figured that out before in the original app DocumentFile code that I didn't notice until now after the fact lol (it was broken use so I didn't check that deep into it). The old code is still there but I'm not going to try to force it over anyway due to issues that would present.

Further, some code has been simplified to minimize time wasted during file transfers and remove stuff causing duplicated or actually unnecessary things.

This has resulted in much better code, much faster transfers (still not anywhere as fast as File however), and improved path and filename use that caused the "a/a a" delete issue.

Non actual app problems encountered:

1. Android Studio messed up build state may cause various issues with using the app including slowness, inability to for the app to start, partial start waiting for debugger, or excessive picker popups. Fully invalidate caches seems like it was the fix. Uninstall/reinstall, clear data/cache, and device reboot did not work.

2. There is actually a performance loss involving Android 14 (Changes needed to target Android 14 #210 (comment)). Solved: This one is actually Android 14 itself and nothing to be done.

3. Others posted below.

ALSO see newest commit post below for other most important info:

• scoped storage fixes and improvements #205 (comment)

Other posts contain less important info but still important.

[Xavron]

Problem seen. Needs a fix :(

...Two problems actually. 1) Regression that caused DocumentFile to be null so that will be fixed. 2) Regression of path related problems on internal.

Since I implemented multi user scoped storage (not yet released) its much easier to test now.

Both are fixed and will be added in upcoming commit(s) at some point. Need to look into 2 further to see if it can be simplified to a greater extent.

Edit - further related code simplifications required and incoming as well.

Fixes added in commit: 97154f7c88477e9111ea4317babd7fd2c6cab01b

[Xavron]

Forgot about the other non app issue. Windows 11 23h2 File Explorer may show incorrect size on disk values in rare file situations. The affected files have contents that match the original(!) and correct size(!) with simply size on disk showing incorrectly as 0. Won't fix as its not an app issue.

Further inspection: Write more into a file that has size on disk val of 0. After a small amount, the value finally shows up as non-0. Back off to original file contents and the size is corrected. Also happens for locally created files. Definite non-issue.

[Xavron]

Non-app issue seen. Client connects, RETR done, then nothing. In this case, this was a USB drive enclosure acting up where the client has a temp folder on there. Since the temp folder was having trouble being used, the client here wasn't coded to deal with that, and stays forever stuck.

Further inspection: Switching the temp folder to a different storage drive fixed the issue. Using another client also proved the app as working.

[Xavron]

Further issues to be fixed with next commit here: Fixes for rename and move with scoped storage.

Fixes added in commit: 97154f7c88477e9111ea4317babd7fd2c6cab01b

[Xavron]

This one remains not fixed.

FileZilla does indeed have some sort of duplication issue with multi connections (default 2). Possibly a race condition. Should definitely be related to threading though. That's really for them to fix in their client. The swiftp code really shouldn't have to waste time dealing with bad code in the client but adjusted upcoming commit to deal with that situation anyway instead of not. So... going down as half and half.

Just before it duplicates things (is all good and fine):

CmdPASV ... D PASV sending IP: ----------
CmdPASV ... D PASV sending IP: ----------
CmdPASV ... D PASV completed, sent: 227 Entering Passive Mode (----------).
CmdPASV ... D PASV completed, sent: 227 Entering Passive Mode (----------).
SessionThread ... D Received line from client: STOR file123
SessionThread ... D Received line from client: STOR file587

Is now duplicated on the two connections (not good as it tries to do the same thing twice):

SessionThread ... Received line from client: MKD dir123
CmdMKD ... D MKD executing
CmdCWD ... D CWD complete
FtpCmd ... D Parsed argument: dir123
SessionThread ... D Received line from client: MKD dir123

Setting FileZilla to max 1 connection gets rid of that issue.

Without protections there in the swiftp code, dir123 would have been created again but renamed as "dir123 (1)" since dir123 was already created in the other connection.

Edit: Another attempt at this:

Possible fix: LastModified is still 0 on the duplicate creation so that can't be used and also means exists can't work either (on the correct dir to block duplicates that way). However, checking before and after for the dir with the duplicate name (having a (1) in it) and then deleting it has worked on a first test of it (needs more testing). So that, if it doesn't exist before but does after, then a duplicate was definitely created and is not a false positive. (It could maybe be possible that another connection could at the same time create one that should exist? This is likely exceptionally unlikely if even possible.)

So far this is the only way that is working in code. Only other way appears to be to fully limit all FileZilla transfer settings to 1. I see nothing else - as its a flaw in how DocumentsContract works here and how FileZilla works here so changes in either of them would work as well.

[Xavron]

Newest commit: 97154f7c88477e9111ea4317babd7fd2c6cab01b

Fully simplified paths. Took the time (a lot more than I ever wanted to spend) to find out exactly what was needed from all the combinations, conflicts of the code, clients in different situations, and including client's with their own bugs causing issues. But, now its done =)

Notes:

• 1. This does not simplify use of chroot "/storage/sdcard/..." nor "/storage/emulated/0/..." for scoped storage due to it being a rather heavy amount of changes needed. These uses for chroot instead get quickly reworked to URI compatible paths when needed.

• 2. Restore pure original getDocumentFile() in FileUtil (no longer needed for Android 11+ scoped storage anyway) as there were too many issues surrounding it between old and new.

• 3. Rename (RNTO/move) has been fixed for scoped storage.

• 4. Certain methods like moveFile() in FileUtil have File blocks above DocumentFile that return wrongly like rename() does where that one always returns false even when it is successful which causes issues with DocumentFile below it. I'm not directly fixing those issues and am instead moving the useScopedStorage() blocks above it in various methods where various things have been seen for avoidance.

• 5. Fully checked against all tests found below.

All tests successful with this code:

Android 8 (Java.io.File) [ALL TESTS PASS]
user [x]
add [x]
delete [x]
edit [x]
chroot changes [x]
WinSCP command line commands [x]
internal [x]
put [x]
get [x]
rmdir [x]
rmdir with files in it [x]
mkdir [x]
cd [x]
cd to multiple subdirs at once [x]
cd .. [x]
cd / [x]
stat [x]
delete [x]
pwd [x]
ls [x]
quit [x]
try to cd out of chroot without success [x]
rename file [x]
rename dir [x]
internal multiple locations [x]
file only [x]
folder only [x]
small folder [x]
medium folder [x]
large folder [x]
7k files in a single folder [x]
sd card [broken as expected and double checked against old build with same results]
clean app install [x]
tested via multiple clients [x]

Android 8 (scoped) [ALL TESTS PASS]
user [x]
add [x]
delete [x]
edit [x]
chroot changes [x]
WinSCP command line commands [x]
internal [x]
put [x]
get [x]
rmdir [x]
rmdir with files in it [x]
mkdir [x]
cd [x]
cd to multiple subdirs at once [x]
cd .. [x]
cd / [x]
stat [x]
delete [x]
pwd [x]
ls [x]
quit [x]
try to cd out of chroot without success [x]
rename file [x]
rename dir [x]
append [x]
sd card [x]
put [x]
get [x]
rmdir [x]
rmdir with files in it [x]
mkdir [x]
cd [x]
cd to multiple subdirs at once [x]
cd .. [x]
cd / [x]
stat [x]
delete [x]
pwd [x]
ls [x]
quit [x]
try to cd out of chroot without success [x]
rename file [x]
rename dir [x]
append [x]
internal multiple locations [x]
file only [x]
folder only [x]
small folder [x]
medium folder [x]
large folder [x]
7k files in a single folder [x]
sd card multiple locations [x]
file only [x]
folder only [x]
small folder [x]
medium folder [x]
large folder [x]
7k files in a single folder [x]
full file verification [x]
all tests with each user for multiple location testing [x]
continual backups to 2 users at internal and sd card [x]
clean app install [x]
tested via multiple clients [x]
tested upgrade from current version [x]

Android 14 (scoped) [ALL TESTS PASS]
user [x]
add [x]
delete [x]
edit [x]
chroot changes [x]
WinSCP command line commands [x]
internal [x]
put [x]
get [x]
rmdir [x]
rmdir with files in it [x]
mkdir [x]
cd [x]
cd to multiple subdirs at once [x]
cd .. [x]
cd / [x]
stat [x]
delete [x]
pwd [x]
ls [x]
quit [x]
try to cd out of chroot without success [x]
rename file [x]
rename dir [x]
append [x]
sd card [x]
put [x]
get [x]
rmdir [x]
rmdir with files in it [x]
mkdir [x]
cd [x]
cd to multiple subdirs at once [x]
cd .. [x]
cd / [x]
stat [x]
delete [x]
pwd [x]
ls [x]
quit [x]
try to cd out of chroot without success [x]
rename file [x]
rename dir [x]
append [x]
internal multiple locations [x]
file only [x]
folder only [x]
small folder [x]
medium folder [x]
large folder [x]
7k files in a single folder [x]
sd card multiple locations [x]
file only [x]
folder only [x]
small folder [x]
medium folder [x]
large folder [x]
7k files in a single folder [x]
full file verification [x]
all tests with each user for multiple location testing [x]
continual backups to 3 users at internal and sd card simultaneously [x]
backup restore validation [x]
clean app install [x]
tested via multiple clients [x]
tested upgrade from current version [x]
transfer (to) multi times same location with manual delete and keep WinSCP dirty (store code conflict not caught by anything above) [x]
tested with signed release build [x]

Other tests not listed for various reasons were also successful.

[Xavron]

No further commits are expected. Code is well tested.

[Xavron]

Unexpectedly, rewriting custom code for DocumentFile LIST in replacement of DocumentFile.listFiles() shows approx 4x improvement in speed. This can be applied to some other code uses and I don't yet know what the total result will be.

Clarification: It is not the speed of listFiles() that is the problem. That would only be a savings not worth anything. This test actually saves about 7 seconds on a 9 second LIST test and puts it somewhere near enough to File performance to be satisfactory. Assuming it works out well in the end.

Update:

Even though as of now its working successfully on 4 automated tests of about 18k files and 1k folders using transfer up and down, and deletions (size and file/folder counts are all exact)... there's still TODOs around.

Full software file verification was also successful with all files a perfect match after transfer up and back down on over 12k files. So, all bytes exact.

I still don't expect this to be a part of this pull request.

Update: From what I can see, this is required to, fix or at least drastically improve, the Android 14 sleep issue. There's only one more comparison test I can think of doing but is expected to yield the same result.

[Xavron]

Decided to test more Android versions while testing battery savings. Seen some issues with the code here in scoped storage needing workarounds or fixes.

Android 9 - Partially tested as good. Needs more testing but should test same success as Android 8.

Android 6.0 - Maybe a scoped storage issue hiding here when no battery optimization is engaged. Need to look into further if its something with the code or the client. Tests completely successfully with enabled OS battery optimization for the app.

Android 7.1.1 - Some random oddities causing various issues that shouldn't be happening but are. Like DocumentFile name being null while its Uri path with the name is fine. 7.1.1 is all weird and crazy here :)P

Fixes to be added in future commit.

Went back further and did longer comparisons with File. Same problems occur with File. This device or Android 7.1.1 is really crazy.

o Even with wakelocks enabled, turn screen off and the test may get stuck at some file, and will then start going again after turning the screen on. So the CPU wakelock that is in advanced is required here in order to keep the screen on (can't turn it off even with it enabled). There's an old reported issue that looks like the same problem that I noticed previously (probably this one: #95 but I'm not seeing a missing port as that person suggests. Its still there, just the transfer doesn't move. No reconnect, no errors, nothing. Just stays put. There might eventually be a timeout after a long time but idk. I do have higher digit client timeout in use which could be why I see a difference.)

o That issue linked right above is also A7 which doesn't speak well to the Android version instead of the device. However, I do wonder why even File has issues. Might be mixed part device or this major Android ver is actually that messed up here?

o Missing files/folders, even with File use during that test as well. Probably wrong size too I'm now expecting even with correct file/folder count but not going to test File to that extent now.

o Device is unusable for this app. But, I will try one further test to double check that. (Test concluded: random failure also with missing files using app build of code from 6/4/2022 confirms the issue is also present in File on this A7.1.1 device. Not seeing an app code issue at this time that would include random failure of both File and DocumentFile when other Android versions are great.)

Not looking into Android 6 or 9 any further. The code changes here of this pull request are working great.

Done here unless maybe its still here in a year :)

Edit: MFMT can't be added as Android provides no way for scoped storage to do this. #215

[Xavron]

shakes tree Unexpected issue: Anonymous use.

Anonymous crashes the app on newer Android versions (aka Android 14). This is actually a problem between scoped, scoped multi user, and anon. To do multi users with scoped, have to be able to pick out the uri from a list. As anon doesn't offer this for chroot selection (its currently static to "/storage/emulated/0"), the code gets bumped to File where Android denies its use, and this crashes the app.

Fixed with pull request: #228

[Xavron]

Commit 4cdd48e to close #188 - fix CmdHASH (checksum)

[Xavron]

Issue seen on a very specific dir deletion in CmdRMD where the path wrongly winds up with a duplicate dir in the path itself eg ".../dir32/dir32/..." in WinSCP. FileZilla is fine so its a client based difference needing to be taken into consideration.

Will, also fix dirs that are at chroot as a sub chroot path with identical names. That's not easy to explain so eg chroot path "/system/emulated/0/testDir/" and creating a dir "testDir" to become an actual "/system/emulated/0/testDir/testDir". Should be valid use.

I don't see any way of simplifying the client conflicts here but will also look into doing that further. Although, I could wind up missing a simplification even looking at this 100x :)P

A fix is now in testing but will take time to make sure its definitely not conflicting as clients here are crazy conflicting and look into further simplifying it as noted above :)

Fixed with commit d2b3db1

[Xavron]

Added c51bdf3 to fix an issue involving REST command use.

Removed and then re-added via commit 396644e to a false problem here. Only conflicted with some currently unknown problem in under construction FTPS code.

[Xavron]

Fix for current last known issue.

Added commit b209ff0 to fix possible app crash on loss of scoped URI permission.

And re-add a method I somehow lost and the above commit removed 82e4cf7 ...I seem to have lost the CmdHASH changes in my own code but at least not here lol.

[Xavron]

I think there's something going on here... don't know what happened as it didn't show up before. But, running into problem after problem in trying to track it down whatever it is :'(

Best to hold onto it until I track this problem down and actually figure out what it is.

But, seeing an issue after transferring to and deletion multiple times in a row of a 12.4GB folder with 251 files and 88 folders. It seems to be throwing me red herrings.

Definitely happening in this pull request branch unless that's another red herring lol. All of the following involved:

• One recent fix above with the issue for it being more picky than I realized.
• REST offset is having a problem correctly getting through STOR it seems.
• offset > 0 when the file is 0 (rfc shows server can send error for this? I think that's what it says.).
• And still something else I've been chasing for weeks where the first STOR only gets to read once. I don't know what's causing this though but now have to look at it yet again.

On the side also: an issue with this one device's wifi, and there was an Android Studio cache issue lol.

• Another note: sessionThread.offset is a bad thing to base code on such as during STOR. Connection issues can see REST as some large num above 0 while the file length is 0. It can also incorrectly steer the code such as in the event a file doesn't actually exist and offset > 0... that's a failure for sure.

• So far looks like a combination of a sub router (using AP mode) stability issue (confirming on a different wifi AP right now but a manual router reboot did stop some issues despite auto reboot function in use) and sessionThread.offset use / REST steering in STOR that is causing possibly most / all of the issues that get it working smoothly. Will see if it keeps going that way. A bit early in tests still.

Fixed with commit d2b3db1

[Xavron]

That was it:

• Confirmed via another wifi AP.

• Some minor adjustments to REST use in STOR for scoped storage.

Tests done. All successful:

• Android 14.

• Plain connection.

• Automated 60k+ files transferred up, down, deletion, and comparison of original and the files transferred back.

• Manual transfer tests up + deletion + repeat a few times back-to-back using same connection for 1 hour. Tested this on both wifi APs.

• Automated used three varied internal locations and one sd card location. Manual used sd card.

• Automated FTP command test of a nice chunk of commands on all three internal locations and sd card.

• Manual transfer of 5k files and 400 dirs of up, down, and then automated full file verification (which includes that everything is in the correct location as well) on the returned transfer to make sure there's absolutely zero difference.

Good to go. All fix commits added and better than ever :) Sigh... see next post lol. At least I'm piling on more things to automated tests and to my other things that I use so ever more is constantly tested.

[Xavron]

Issue seen. Hash via WinSCP synchronize is having a problem with path.

I don't know if this is affecting java.io.File at this time... although it looks like it might be as I'm not seeing the scoped storage code get used here in making it. But, creating this issue here as that's where I'm testing and its been a long day.

eg (CmdHASH param): HASH "/test/ftp app test folders.zip"

becomes double quoted at String param = getParameter(input);

""/test/ftp app test folders.zip""

Path then gets a quote char in it at fileToHash = inputPathToChrootedFile(sessionThread.getChrootDir(), sessionThread.getWorkingDir(), param); and test dir is wrongly duplicated.

.../test/"/test/ftp app test folders.zip" .................. Bad path so checksum fails to find the file.

I should commit a fix within a day or so.

Fixed with commit ab048d5. Conflict tested on Android 8 and Android 14 via scoped storage. Thousands of files transferred up down and deleted via auto tests with full hash testing on every returned file compared to original. Various command auto tests also run successfully. Briefly tested against java.io.File for confirmation.

[Xavron]

Testing STOR performance. For reference.

46.300  D  STOR/APPE executing with append = false
46.301  E  Speed test 0 ............................................(before all)
46.301  E  Speed test 1 ............................................(before chroot check)
46.302  E  Speed test 2 ............................................(after chroot check and 2 next checks)
46.303  E  Speed test 3 ............................................(after exists check)
46.304  E  Speed test 4 ............................................(before mkfile)
46.382  E  Speed test 5 ............................................(after mkfile)
46.390  E  Speed test 6 ............................................(before output stream)
46.422  E  Speed test 7 ............................................(before bytes loop)
47.101  D  Returned from final read
47.106  D  STOR finished

16.500  E  speed test mkfile before all
16.510  E  speed test mkfile before create
16.573  E  speed test mkfile after create

documentFile.createFile() should be where there's a large performance loss compared to java.io.File. I didn't do a comparison with this test but looked into it a while back.

This also occurs with DocumentsContract.createFile().

java.io.File use is denied here on Android 14 and earlier Android versions could see a speed up where java.io.File use is permitted for this like Android 8 internal. Would only be a workaround but it could be done.

May not be able to get any further meaningful performance out of STOR with scoped storage code as it exists today. There may also be other things here that nothing can be done about.

One possibility could be to cache the stuff in memory and write it to storage on another thread but this would cause problems with following ftp commands. So, not a great idea here. That would get far too complex to deal with. Quick memory only test shows about 87ms saved on this single file. That's 1 second saved for every 12 files. 15 minutes saved for about 10.8k. However, can more easily throw better wifi, CPU, RAM, storage, at it for meaningful speed gains.

Speed gains from #205 (comment) (however it might forever stay as unreleased or experimental due to its nature and as well is not yet finished) can potentially heavily impact time saving when storing multiple files and dirs as MLSD use is seen when dirs are queued. Not speed tested against storing files in the way done here at this time. Also used are CWD, MKD, and PWD when dirs are involved. Only MKD and MLSD could have anything meaningful for saving time. MKD is unlikely to see any speed gains as in the way of createFile().