Stub databases should be read with msvc_posix_open

[Richard Boulton]

Currently, stub databases are read using a standard C++ ifstream. (See backends/database.cc, function open_stub()) This works fine, except that if a user (or the database replication code) tries, on Windows, to atomically rename a new stub db file over an existing one, it will receive an error if the old stub DB file was open.

This can be avoided if we instead use msvc_posix_open() (or just open() on unix) in open_stub() to get a file handle for the stub database, and access it using C file-handling routines.

[Olly Betts]

hmm, but if we do this, what happens on windows if the file is overwritten while we're reading it? The read will fail I believe...

[Richard Boulton]

So, a full fix would probably have to handle failures of the read, and retry. What a pain.

[Olly Betts]

Ah yes, that should work if the read returns a suitable error (which I think it does).

[Olly Betts]

For MSVC it looks like we could just write:

ifstream stub(msvc_posix_open(file.c_str(), O_RDONLY));

But GCC's libstdc++ doesn't have this non-standard form, so we can't use this for mingw, but there is stdio_filebuf, which allows you to wrap an fd or FILE* as an istream or ostream. This would allow us to keep the current code with only minor changes on the affected platform.

We should perhaps check if iostream imposes an overhead over the C FILE* routines, and if there is much of one have a policy to avoid istream and ostream instead.

Also while one option is to retry the whole stub read upon read failing, another is to simply require that the stub update attempt retries. It is perhaps better to have the writer blocked by heavy reader activity than have readers blocked by heavy writer activity as readers tend to be more performance sensitive.

[Olly Betts]

Milestone renamed

[Olly Betts]

While looking at fixing #854, we ideally want to read the stub file from a file descriptor. In the case where we've been asked to open a database passing in a path which is a file and that file is large enough that it could be a single file database (which means at least 2KB for glass and honey) we open an fd on it to look at the file magic to see if it is, and if not we currently close the fd and open the file again as a std::ifstream.

That means there's a race condition if you replace a >= 2KB stub file with a single file database - if you're very unlucky with the timing, a reader could sniff the stub file, close it, another process updates the filename to be a single file database, and the reader opens that as a std::ifstream and tries to parse it as a stub file.

I think there's also a race for a smaller stub file - as we could stat() the stub file, another process updates the filename to be a single file database, and the reader opens that as a std::ifstream and tries to parse it as a stub file. We should always open as a file descriptor first and use fstat() on that to decide if we can skip checking the file magic.

(It's also slightly inefficient to open the file twice, though I suspect most stub files are under 2KB so currently only get opened as a std::ifstream.)

That requires moving to reading stub files from a file descriptor, so as part of that we can address this issue by using posixy_open() (which replaced msvc_posix_open() back in 2012, and is just an alias to open() on Unix-like platforms). We probably just need to ensure that all reading of stub files happens this way (there's some in replication for example; ideally they should probably share more code anyway).

[Olly Betts]

Fixed by add5c798b671e470be8e09d910463d193d3b6a0e.

So, a full fix would probably have to handle failures of the read, and retry. What a pain.

I've not attempted to handle retrying reading the stub file in xapian-core. Perhaps we should, but at least for now user code will need to handle the retry.

[Olly Betts]

So, a full fix would probably have to handle failures of the read, and retry. What a pain.

I had a quick poke at this in CI: ​https://github.com/ojwb/test-stuff/actions

It seems with modern Microsoft Windows versions that the file remains readable after deletion. I found this interesting comment which appears to be from someone at Microsoft: ​https://github.com/golang/go/issues/32088#issuecomment-502850674

in the most recent version of Windows, we updated DeleteFile (on NTFS) to perform a "POSIX" delete, where the file is removed from the namespace immediately instead of waiting for all open handles to the file to be closed. It still respects FILE_SHARE_DELETE, but now otherwise behaves more like POSIX unlink. This functionality was added for WSL and was considered worth using by default for Windows software, too.

I have to say it's refreshing for Microsoft to be making things more compatible rather than less!

Some background to that comment is that some places suggest you should rename a file to a random name (and probably make it hidden) before deleting it because on older OS versions the file actually continues to exist for a while and that causes issues if you try to create a file with the same name as the deleted one. I've seen suggestions to rename into a higher level directory to avoid blocking the removal of the containing directory. Creating randomly named hidden files in the root directory of a volume seems really icky though.

That comment doesn't explicitly say, but my testing shows that open handles also continue to work on such a deleted file. Things may be less rosy when not using NTFS though - I don't think I can easily test that via GHA.

I also tested atomic rename in that repo's CI. It seems this fails with ERROR_ACCESS_DENIED if there's an open handle on the target, even if that handle was opened using the posixy_open() approach. This affects using posixy_rename() to atomically update files, but it's the writer not the reader that is affected. I think this is actually more problematic that the stub update problem as it affects the mechanism we use during WritableDatabase::commit() to update the version file. Experimenting it seems we can solve this by having posixy_rename() first try to use ReplaceFileA() which works even if the target is open; it fails unless the target already exists, so we then need to fall back to MoveFileEx(). (This doesn't work under Wine for some reason, which we use for CI - I think we don't exercise the case that doesn't work though.)

Before I found ReplaceFileA() I tried doing _unlink() before posixy_rename() which fails in the same way under Wine but seems to work on the real platform. It would be problematic to do this as it means the replacement isn't atomic as there's a window when the target is missing, but I thought it interesting to try.

I think this is much improved yet still not perfect. The only obvious thing I can see we could do is rename to a random hidden name before deletion, but it seems that's not needed on modern OS versions (at least on NTFS).

I'll add a comment noting that to the source code, but it's such a fight to get the behaviour we want on this platform that I don't think I'm going to spend more time on this unless/until we get feedback from people actually using Xapian on Microsoft Windows and actually hitting problems.