Add more functionality to LLFile and cleanup LLAPRFile

[fmartian]

Description

This patch adds LLFile::size(), LLFile::read() and LLFile::write() static functions and replaces all calls to various LLAPRFile functions with the according functions from LLFile.

LLAPRFile::remove() replaced with LLFile::remove()
LLAPRFile::size() replaced with LLFile::size()
LLAPRFile::isExist() replaced with LLFile::isfile()

The according LLAPRFile functions are retired together with the already unused LLAPRFile::rename(), LLAPRFile::makeDir() and LLAPRFile::removeDir() functions.

Also cleans up remarks about thread safety of used APRPools where that is not relevant anymore since the according LLFile functions do not require a special memory pool.

Related Issues

• [*] Related to the discussion in Add Memory Map and 64 bit file support to LLAPRFile class #4566

Checklist

Please ensure the following before requesting review:

• I have provided a clear title and detailed description for this pull request.
• The PR is linked to a relevant issue with sufficient context.
• I have tested the changes locally and verified they work as intended.
• Code follows the project's style guidelines.
• Documentation has been updated if needed.
• I have reviewed the contributing guidelines.

Additional Notes

Next step will be to add non-static methods to the LLFile class, and make it a full RAII class in terms of the internal resources managed in that way.

[Ansariel]

Wouldn't it be smarter to use standard filesystem library at this point instead of replacing platform independent code provided via 3p with platform specific code and API calls?

[akleshchev]

Agree, using std::filestystem where possible would be nicer. It should be mostly functional now that minimum MacOS version was bumped to 12. But personaly I would be happy with having less APR regardless (we have been planing to remove the thing for years, but it's just too entrenched).

[fmartian]

Agree, using std::filestystem where possible would be nicer. It should be mostly functional now that minimum MacOS version was bumped to 12. But personally I would be happy with having less APR regardless (we have been planning to remove the thing for years, but it's just too entrenched).

With my last changes, use of the APR library is pretty much limited to the cache implementation and only very few other places. The cache is a somewhat daunting thing to replace with different functions. A solution that works 99% of the time is not enough for this. The rest seems pretty trivial to chuck once there is a working alternative, and very low risk.

Rather than waiting for std::filesystem to be fully functional it seems actually easier to extend LLFile with the necessary methods. I already have a halfway working implementation of a simple LLFile class instance with open(), seek(), tell(), and read() and write() methods. As long as the desired support for platforms is limited to Windows/Mac/Linux, it's not that a difficult exercise.
It even allows share locks, although on non-Windows it only provides advisory locks using the flock() function. Not perfect but enough to provide a means to let multiple viewer instances lock specific files from each other. Other applications with the necessary access rights but not using flock() still can clobber the files, but that is something that is almost impossible to prevent on non-Windows systems.

I did on purpose not create an all or nothing solution but wanted to do this step by step, starting with the more easy functions operating on just filenames.

But replacing the rename, remove, size and similar calls with std::filesystem methods may indeed be an option. I'll have a look at that. Still think that a fully RAII capable LLFile may be useful for normal file access. I'll check what std::filesystem can provide here and do some tests.

[fmartian]

Agree, using std::filestystem where possible would be nicer. It should be mostly functional now that minimum MacOS version was bumped to 12. But personaly I would be happy with having less APR regardless (we have been planing to remove the thing for years, but it's just too entrenched).

There is one rather implicating point about using std::filesystem functions: They tend to throw exceptions on pretty much any underlaying error. The current viewer code is definitely not prepared to deal with that in most places, so replacing functions like LLFile::rename() or LLFile::remove with the according std::filesystem functions is going to be a fairly substantial refactoring exercise. Anyone willing to venture into that?

[Ansariel]

There is one rather implicating point about using std::filesystem functions: They tend to throw exceptions on pretty much any underlaying error.

They only do if you use the throwing version of the methods. Each method is overloaded and take a std::error_code parameter - those are non-throwing.

The current viewer code is definitely not prepared to deal with that in most places, so replacing functions like LLFile::rename() or LLFile::remove with the according std::filesystem functions is going to be a fairly substantial refactoring exercise. Anyone willing to venture into that?

The idea was to use std::filesystem inside LLFile, not replacing LLFile everywhere.

[fmartian]

There is one rather implicating point about using std::filesystem functions: They tend to throw exceptions on pretty much any underlaying error.

They only do if you use the throwing version of the methods. Each method is overloaded and take a std::error_code parameter - those are non-throwing.

The current viewer code is definitely not prepared to deal with that in most places, so replacing functions like LLFile::rename() or LLFile::remove with the according std::filesystem functions is going to be a fairly substantial refactoring exercise. Anyone willing to venture into that?

The idea was to use std::filesystem inside LLFile, not replacing LLFile everywhere.

Thanks for the nudge into the right direction. I was initially seriously considering to throw out LLFile too and directly call std::filesystem and co, but that would be a real mess to manage.

So it appears that use of std::filesystem for the path related operations would be a viable option and I kind of like the noexcept variants of that interface. There are slight adaptions of behavior necessary mostly related to the viewers preferences to treat some errors not as an error at all and create_directory() does not have a permission mask, which is a minor inconvenience since the only place it is used is an obscure dir_exists_or die() and according to a comment inside, this function is only used in the simulator, which I don't think is any concern for the current viewer codebase. Even if it was, that call uses the current default value too, so not sure what relevance it has at all.

For a sane file read/write access to fully replace APR we would then have to use ifstream/ofstream and/or fstream to make LLFile a thin wrapper around this interface. The only three drawbacks I see with this:

1. There seems no public stat() like functionality in the STL
   The viewer uses this in several places to not only detect if a file or directory exists but to also then use the st_ctime, st_mtime, and st_size. The STL provides functions to return the two last values but it will do the whole string->path->openfile->stat->closefile each time. There seems no way to retrieve the st_ctime through this interface.

2. No support of long paths under Windows
   The STL maintainers have repeatedly refused to make the Windows STL implementation long path capable mainly on the grounds of backwards compatibility and their believe that they can not implement a fully safe path normalization, which would be mandatory since the long path name prefix bypasses most kernel interface path normalization and sanitation measures and passes the path unchecked to the kernel itself. This is relatively harmless in an environment where users can't directly enter full pathnames, but the STL library doesn't have the comfort of such a guarantee.

3. No shared/exclusive file locking on open
   This would be a very handy feature as it makes management of concurrent access to files from different processes a lot easier. The fact that it is only advisory on non-Windows systems isn't a killing argument, as it still would allow to manage concurrent access to files from multiple viewer instances. The native_handle that could make this possible to call the according platform function is unfortunately only a C++26 feature, so a nogo here.

And fstreams error handling is a bit awkward, not in any way similar to the more modern std::filestream error handling. Yes we could enable exceptions() on the fstream, but that does not make for cleaner code either and adds quite a bit of complexity in getting it exactly right.

[akleshchev]

So it appears that use of std::filesystem for the path related operations would be a viable option and I kind of like the noexcept variants of that interface.

Viewer already uses std's existance check in some places (not sure why), can act as an example.

and according to a comment inside, this function is only used in the simulator

You can safely ignore that. There is no mechanism to sync viewer/simulator base even for llsd, much less LLFile.

No shared/exclusive file locking on open

That's the bigger problem with replacing APR's file access (but APR as a whole has some additional issues with network and processes).

P.S. Also note that some places use boost::filesystem. Some of that is safe to replace with std::filesystem and boost can pad missing std support in other places (as not everything is supported on mac). boost::interprocess::file_lock might be a compensation for APR's locking.

[fmartian]

So it appears that use of std::filesystem for the path related operations would be a viable option and I kind of like the noexcept variants of that interface.

Viewer already uses std's existance check in some places (not sure why), can act as an example.

I think it would be useful to make the whole viewer all use the same function, most probably the LLFile wrapped function.

No shared/exclusive file locking on open

That's the bigger problem with replacing APR's file access (but APR as a whole has some additional issues with network and processes).

The file_lock is currently only used in one place really: llappviewer.cpp for the marker file itself.
It might be possible to hack that sort of into. Accessing the native handle of an existing fstream is not an option as that is only a C++26 feature, but a possible workaround might be to create a file descriptor in the old way and wrap it in a stream_buf. The possibility seems to exist but it is not really part of the official standard. That way we would have access to the underlaying file descriptor and can do flock() on that (or LockFileEx() on Windows). But it would probably be an option in the open() call, not a separate method.

P.S. Also note that some places use boost::filesystem. Some of that is safe to replace with std::filesystem and boost can pad missing std support in other places. boost::interprocess::file_lock might be a compensation for APR's locking.

boost::filesystem is basically the origin of what eventually got std::filesystem but with adaptions. So does boost::filesystem try to support the long path name prefix, although I'm not sure it fully can handle it. It at least does not bork the path when you try to determine the root of a path that has the Windows NT kernel object space prefix. More functional IMHO would be to keep that completely out of the actual path layer and to simply apply that prefix whenever a path is approaching the MAX_PATH limit before passing it to the WinAPI wide char function. But that does require proper path sanitation and while I did some of that in the old implementation it is not really enough to sanitize random user entered paths.

[fmartian]

I'm running into some rather annoying problems trying to run the unit tests.

The first problem are the annoying firewall popups that appear for every other test or so. There must be a solution for this!

Then the tests randomly seem to get stuck after reporting success. They typically start, do their thing report that they happily succeeded but never quite terminate. By far not all but enough to be a real nuisance. After 4 or so such stuck tests, the Visual Studio process scheduler has exhausted its pool and sits just idling and waiting for one of these to finally return. The only thing that helps is to go into Task Schedular and terminate the child process for these stuck tests, which of course causes the batch wrapper to report an error, but lets Visual Studio continue with launching more tests until it exhausts its pool again.

[akleshchev]

You should be able to run specific tests individually instead of all of them. Never had that issue with firewall or tests being stuck, don't have any ideas how to fix that.

[fmartian]

You should be able to run specific tests individually instead of all of them. Never had that issue with firewall or tests being stuck, don't have any ideas how to fix that.

That's my plan but I found a few other tests that depend directly or indirectly on LLFile and need to run them too, to be sure. One specific change to remove rmdir() since remove() will happily do that now too, did actually break compiling for 3 or so tests. Rather than trying to remember what all could break in the myriad of tests and how to look for these occurrences it seemed easier to just build them and run them all, except it isn't ;-(

The Firewall dialog is maybe not the actual issue. I couldn't so far establish a relationship between the tests this dialog popups for and the processes that get stuck on exit. In fact there are tests that terminate cleanly and terminate properly where this dialog actually appears, long before I get a chance to acknowledge the dialog. But those dialogs for sure are very annoying and defeat any unit test run attempt as they require you to be present at the machine to acknowledge them.

And that process hang after the test has successfully ended, really irks me. But it reminds me vaguely on a similar issue when running external tools in Visual Studio through a batch file that could make them sometimes hang after process termination. Will try a command line run of the whole autobuild build command as next step.

[Copilot]

Pull Request Overview

This pull request refactors file operations by migrating from LLAPRFile to LLFile static functions, eliminating the need for APR memory pools in file operations. The changes add new static methods LLFile::size(), LLFile::read(), and LLFile::write(), and replace all calls to deprecated LLAPRFile functions throughout the codebase.

Key changes:

• Added LLFile::size(), LLFile::read(), and LLFile::write() static functions with cross-platform implementations
• Replaced LLAPRFile::remove(), LLAPRFile::size(), and LLAPRFile::isExist() with LLFile equivalents across viewer code
• Removed deprecated LLAPRFile functions including remove(), rename(), size(), isExist(), makeDir(), and removeDir()
• Cleaned up thread safety comments that referenced APR pools, which are no longer relevant
• Improved Windows-specific implementations using native Windows APIs (CreateFileW, DeleteFileW, RemoveDirectoryW, MoveFileExW) for better performance and long path support

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
indra/llcommon/llfile.h	Added declarations for size(), read(), and write() static functions with documentation; reorganized comments and fixed typedef formatting
indra/llcommon/llfile.cpp	Implemented new size(), read(), and write() functions; refactored get_fileattr() to return status code via parameter; improved Windows API usage in remove(), rmdir(), and rename()
indra/llcommon/llapr.h	Removed declarations for deprecated LLAPRFile static functions
indra/llcommon/llapr.cpp	Removed implementations of deprecated LLAPRFile::remove(), rename(), isExist(), size(), makeDir(), and removeDir()
indra/newview/llvocache.cpp	Replaced LLAPRFile::remove() and LLAPRFile::isExist() with LLFile::remove() and LLFile::isfile()
indra/newview/llviewerassetupload.cpp	Replaced LLAPRFile::size() with LLFile::size() with explicit cast to S32
indra/newview/lltexturecache.cpp	Replaced all LLAPRFile::size(), LLAPRFile::remove(), and LLAPRFile::isExist() calls with LLFile equivalents; removed APR pool thread safety comments
indra/newview/llappviewer.cpp	Replaced LLAPRFile::isExist() and LLAPRFile::remove() with LLFile::isfile() and LLFile::remove() for marker file operations

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull Request Overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[akleshchev]

It looks like you are adding lock support? Is it supposed to be here?

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 10 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[fmartian]

It looks like you are adding lock support? Is it supposed to be here?

Yes, as I was adding functionality to actually make LLFile also be a class that can represent a file and handle file read/write operations in itself in order to replace more LLAPRFile functionality, I had the choice to just do a simple file read/write/seek interface or also include lock support in the open() method. It seemed easier to me to just add that add the same time than having to revisit these functions later on.

It will be required for some LLAPRFile replacements.

[akleshchev]

It will be required for some LLAPRFile replacements.

I'm aware of that. It's just that amassing changes makes it harder to push the PR, as not only it is unclear when it's ready, but also becomes harder to track history wise (in case some issues surface).

[fmartian]

It will be required for some LLAPRFile replacements.

I'm aware of that. It's just that amassing changes makes it harder to push the PR, as not only it is unclear when it's ready, but also becomes harder to track history wise (in case some issues surface).

I understand that. Should I remove it? I'm busy to incorporate the CoPilot changes (some I already had done previously) and push that to the branch. I can try to remove the lock support for now but am afraid that it will not help the patch size at all. Most of what CoPilot reports is cosmetic but that last suggestion in lldir.cpp for sure is something that slipped through my checks despite doing quite a bit of tests.

If we get this eventually committed, I can work on removing more of LLAPRFile and hopefully won't have to revisit LLFile to heavily for that. I've got some of that already in a different local branch but want to wait adding that.

[akleshchev]

Should I remove it?

No need, might as well handle it since it's already here. I was just going to merge this, but noticed those new changes and that they weren't mentioned in the header. I'm resting today, but will try to doublecheck that on monday, write a test plan then merge.

Thank you for your work!

[fmartian]

Should I remove it?

No need, might as well handle it since it's already here. I was just going to merge this, but noticed those new changes and that they weren't mentioned in the header. I'm resting today, but will try to doublecheck that on monday, write a test plan then merge.

Thank you for your work!

Please hold back until tomorrow. I was going through the Copilot changes and have a few more which are mostly documentation changes that I was planning to commit today, when I found out about the new Copilot remarks. As mentioned some are already addressed in the changes I was planning to commit, but some were not, and that last comment from Copilot for sure is an issue to fix before committing everything. Currently compiling everything and going to run some tests.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 15 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[fmartian]

Please wait another day with merging this to develop. I'm in the process of working on the comments of Copilot, and want to do some more tests about the issues with LLFile::size() not returning -1 on failure. I did that because the viewer seemed to be unable to load the server baked avatar with that enabled. And despite looking at all the places this was called I could not easily pinpoint where it would go wrong with this. Need to test this aspect a bit more.

[akleshchev]

There should be a build fix for that failure in develop within couple hours.