transactions can fail on windows if searches are happening.

[Richard Boulton]

(Probably a bug in the Quartz backend too, and applies to all Windows platforms.)

If searches are being performed at the same time as modifications, transactions will sometimes fail on windows.

The Flint backend removes the .base[AB] file for the "old" revision before making an update to each table. On windows, this can fail if the .base[AB] file being removed is currently open by another process. This may be the case if a separate process is searching the database, and happens to be opening the database at the same time as the search. This will cause the modifications to fail.

Further, the database can then be left in an state in which flint cannot open the database for searching or indexing. We observed this happening in a database in which the record.baseB and termlist.baseB files had been removed. I'll file a bug about this specific issue when I have more information.

I attach a patch containing a proposed resolution to this problem. It's untested (since I don't have a windows machine), and is a crude attempt at a fix: it risks entering an infinite loop if permission to remove the file is genuinely denied. Charlie: can you test this patch and see if it compiles. We need to work out a reproducible test to check this behaviour.

[Richard Boulton]

Crude patch to work around problem

[Olly Betts]

Hmm. as far as I can see, this just isn't possible to fix decently with the current scheme of base files. We only open the base file to read it, so we can't have it open for much less time. So unless windows has so way to say "open this file, but let it be deleted", something like the proposed patch is the only way.

As well as the "infloop on permission denied" problem, under heavy search load a writer can be stalled indefinitely (or has to fail after some timeout).

Or can we lock the base file from the writer such that readers will fail to open it, then delete the file while it's still locked?

If we're going to use the proposed patch, I think we need to know if cygwin is affected - if not, it would be good to avoid using this kludge there. I know they managed to provide more UNIX like semantics for some file operations. And if it isn't affected, it's interesting as it may show how we can avoid this issue.

[Olly Betts]

Assuming that unlink() is a thin wrapper around DeleteFile(), then this problem probably won't manifest on 95/98/Me:

​http://msdn2.microsoft.com/en-us/library/aa363915.aspx

which says:

"The DeleteFile function fails if an application attempts to delete a file that is open for normal I/O or as a memory-mapped file.

Windows Me/98/95: The DeleteFile function deletes a file even if it is open

for normal I/O or as a memory-mapped file. To prevent loss of data, close files before you attempt to delete them."

However it then says:

"The DeleteFile function marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED."

Which confuses me a little (how can it delete on close when you can't delete an open file?)

Anyway, perhaps calling DeleteFile() and the GetLastError() on failure would enable you to discriminate between "permission denied" and "someone's reading the file"? The documentation doesn't say much about what GetLastError() returns but you could just try the two cases!

[Olly Betts]

It appears MSDN is just wrong about DeleteFile on 95/98/Me and this will affect those versions too:

​http://www.cygwin.com/ml/cygwin-developers/2003-10/msg00038.html

However, from the same thread:

​http://www.cygwin.com/ml/cygwin-developers/2003-10/msg00039.html

"However, DeleteFile on NT has a POSIX-like semantics as long as files are opened with FILE_SHARE_DELETE access, which is true for all files opened by Cygwin processes. POSIX-like semantics means, DeleteFile deletes when the last handle to the file is closed, an `ls file' fails with "No such files...". That's not different from the DELETE_ON_CLOSE method. Since DeleteFile is needed for 9x anyway, why bother with DELETE_ON_CLOSE? However, if you can find any situation where DeleteFile doesn't work but DELETE_ON_CLOSE works, that's fine. I, for one, couldn't find such a situation when testing. But of course I can easily have missed one."

So if we always accessed the base files using win32 api calls (ick) and passed FILE_SHARE_DELETE when opening, then we could probably fix this for the NT family (and realistically, we can probably just say that the 95 family isn't supported at this point). Also this suggests cygwin won't be affected.

Anyway, I'll stop worrying about this since I don't have access to the platform, and get on with something useful!

[Charlie Hull]

Richard's patch compiles fine and may solve our current problem...I'll look into how much of flint_table.cc would need to be changed to use Win32 API calls.

[Charlie Hull]

I've now got a patch that uses Win32 API calls instead of open() and unlink(). However this is going to need some *serious* testing and I'm not enough of a Xapian or Python expert to write the test code...maybe we should think about what is a good test?

[Charlie Hull]

Patch to use Win32 system calls to fix the problem

[Olly Betts]

The first thing to check is that "make check" works. That doesn't really test concurrent search/indexing, but if that fails you've got bigger problems!

Hmm, except if this is for MSVC, I guess you don't have "make check"? If not build apitest and run that (with "-b flint" will save some time).

Also, if you haven't already, it would be useful to check the scheme actually works with a little test program which tries to delete a file which another test program has open for reading.

Other than that, can you test this in the environment where the problem was originally observed?

But overall, if you're confident the problem is fixed and the patch doesn't risk breaking other platforms or make the code significantly harder to maintain, it's OK with me. I'd like to get rid of the base files in the longer term, though I think we may need to keep at least one small file per database which is handled in this way. Perhaps this can be done such that this issue can be avoided though.

[Olly Betts]

The patch looks pretty good, though please use TAB=8 spaces and use the same conventions for whitespace around brackets and braces. It's really good that we can use standard file descriptors except for where we open and delete files.

However, you've patched the wrong place! Those open calls aren't used by the base file (and we don't need to be able to delete the ".DB" file while it is open so there's probably no point patching those). There are two calls to "open" in backends/flint/flint_btreebase.cc which are the ones you want to change.

[Richard Boulton]

Patch which adds a test for this bug

[Richard Boulton]

I've attached a patch to this bug which adds a test case. On Linux, this test passes - if my theory about what is happening is correct, it should fail on windows.

The test case works by creating two threads - one of them repeatedly opens a database for reading, while the other adds 1000 documents to the database, flushing after each document.

Charlie - please test this on windows, and let us know what happens. You'll need to link the test case against Xapian and the Xapian testsuite (in the same way as apitest is currently linked against the testsuite). You'll also need to link it against a pthread implementation.

Before being applied to the xapian tree, we'll need to modify the patch not to use pthreads (it could use fork(), probably), or add pthread detection code to the configure.ac file. I'm not sure it's of much value on Linux at present (though it would be good to expand it to do more general tests of Xapian's stability under heavy load).

[Olly Betts]

[I just got a "mid-air collision" with Richard's comment - it looks like you're aware of the pthread issue, but I'll post this anyway as it contains useful information]

Looking at loadtest.cc, if opening for read can fail with DatabaseOpeningError because of anything a writer is doing, I think that's a bug! Failing to open with DatabaseModifiedError is OK, though somewhat regrettable.

Requiring pthread could be irritating. There's an "ACX_PTHREAD" macro around which probes for what's needed, but it requires you to use a link line of the form:

$PTHREAD_CC $CFLAGS $PTHREAD_CFLAGS $LDFLAGS ... $PTHREAD_LIBS $LIBS

That is problematic since we want to use a C++ compiler, and want to link using libtool.

Possibly this can be fixed by hacking the macro, but maybe using fork() on Unix is the path of least resistance - you only seem to need 2 threads.

[Charlie Hull]

OK, I've been trying Richard's load test under Windows, and I have been getting null pointer exceptions (note, this is using unpatched code, i.e. I have removed any Win32 stuff I wrote earlier today). I've thus started running the code under the Visual C++ debugger, and the first (probably of many) issue is a null pointer at line 308 of btree_base.cc:

buf.append(reinterpret_cast<const char *>(bit_map), bit_map_size);

bit_map is a zero value. This is when a database is first being created in the load test code, at line 107 of loadtest.cc

Any ideas? If I put a simple 'if(bit_map==0) then skip this line' then it works (at least for a while, the load test runs a few cycles then fails somewhere else)

[Charlie Hull]

Sorry, red herring, this is using Quartz when I should be using Flint....

[Olly Betts]

Still shouldn't happen, though bugs in quartz are of less interest at this point as it won't be the default in 1.0. But currently flint is pretty similar in this area, so you may see this with flint too.

I had a quick look at quartz, and assuming no memory corruption type issues, this means it's trying to write the bitmap before it has been read.

[Charlie Hull]

Yes, exactly the same happens at line 321 of flint_btreebase.cc. I'll carry on investigating.

[Richard Boulton]

Patch to avoid double frees if transactions fail

[Olly Betts]

I feel we ought to have a regression test for a bug like that. Do you have something suitable, or a larger example I could cut down?

[Richard Boulton]

Test script simulating failure seen on windows

[Charlie Hull]

Excellent, Richard; your patch is exactly where the debugger is showing errors. I'll apply it and try again. Remind me, why are both of us working on a Saturday? ;)

[Charlie Hull]

OK, I've now got it failing to delete a file (sys_unlink in flint_table.cc) while under heavy load, as expected. Applying Richard's original patch ('wait-a-bit-then-try-again') fixes the problem in debug and release builds (the latter is probably thrashing it harder). I'm testing my own patch using DeleteFile which is probably safer in the long run, but at least we have a fix. Thanks guys!

[Charlie Hull]

More information; failing the delete causes an ERROR_SHARING_VIOLATION, so Windows is behaving as we expected, not letting you delete a file that another process has open. We can cure this by using Win32 API calls to create the file, marking it with FILE_SHARE_DELETE; I have tried this and it works. I'll post a final set of patches soon.

[Richard Boulton]

Olly - Re: regression test. The Python script I've attached (comment #19) causes the memory errors to occur (running it under valgrind shows them clearly, if you have a python build specially compiled to run with valgrind). At some point (maybe later today, though probably not until monday), I'll convert it to a proper test case.

The script also exhibits a further problem with Xapian's transaction code - in that it fails to recover from a failed transaction, and ends up corrupting the database (ie, it results in a database which can't be opened because it has no consistent set of base files). Obviously, this could happen in other circumstances than the windows specific situation this bug report is about, so this needs further investigation and fixing - once the python script has been converted to a normal test case, it should be reasonably easy to fix. As an experiment, I tried changing the "sys_unlink()" call in flint_table.cc to "sys_unlink_if_exists()", and this seems to fix the problem (in that the transaction which the python script simulates failure for is rolled back, and future transactions proceed as normal). However, I'd not want to apply that change without getting a proper understanding of what is going on: obviously, we need to cope with transactions failing in all sorts of ways.

[Charlie Hull]

Patch to create files that can be deleted while open (Windows only)

[Charlie Hull]

Patch to create files that can be deleted while open (Windows only)

[Olly Betts]

Sigh, the formatting is still just as wrong.

A tab should be 8 spaces, not 4. Just set that in your editor and you should be fine (and the existing code will make a lot more sense too!) And follow the conventions for whitespace (horizontal and vertical) around brackets used in the rest of the code.

If you're really unable to fix it, I guess I'll have to, but I'm in the middle of other things so it might take me a while to get to it.

Otherwise, it looks OK. There's a pointless assignment "h = -1;" on failure to open which should be removed.

Presumably the same issue applies to quartz too - are you planning to port the fix across?

[Charlie Hull]

Patches both Flint and Quartz against Windows delete bug

[Olly Betts]

I've applied Charlie's latest patch. I tweaked the formatting a little and conditionalised the inclusion of safewindows.h so the Unix build still works so it would be useful to check it still builds with MSVC.

I'm going to close this bug now and open a fresh one for the other issue (that flint gets unhappy if you delete some of the base files, which the delete problem on windows was tickling). That way we can track it using a bug with just the relevant information in. I'll subscribe you guys to the new bug.

[Olly Betts]

This bug will be fixed in 0.9.10 (coming soon).