Reduce lock scope in FSDirectory.DeleteFile, #1428

[marionoack]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Related to #1428 hanging process on File.Delete

Description

I found (with help of AI) two places where locking behaviour for Fails on file.delete could be improved. I extract the relevant part of it and would be happy, you can integrate it too.

[paulirwin]

Thanks for taking the time to report the issue and submit the PR!

The FileShare.Delete being missing is already a known issue that is being fixed in #1373 (the MMapDirectory overhaul)... and related to that, of note, MMapDirectory currently has a nasty performance issue (see #1151) among other problems. I would recommend switching to use SimpleFSDirectory, which already uses FileShare.Delete, and despite its name, will likely perform better (especially under heavy load) than MMapDirectory does today.

I think your proposed change to narrow the locking to just around the stale files hashset is reasonable; upstream doesn't synchronize around this so I don't particularly see a problem with it. I'll run it through its paces and let you know.

[marionoack]

fixed all request with commit 6cfabf1

[paulirwin]

Thanks for jumping on the feedback so fast. I investigated this further, and I think the diagnosis is a bit different than how it first appears.

Per your stack trace, the thread is blocked inside Win32Native.DeleteFile, waiting in the kernel. If it were a file sharing issue, you would see a sharing violation instead. A thread parked indefinitely in DeleteFile means the SMB redirector is waiting on a network round-trip, most likely to break a lease on the file. This is consistent with the other things you observed in the issue.

Switching to SimpleFSDirectory would be a good diagnostic to see if it helps, but is likely not the proper solution. Regardless, it should be attempted due to likely performance gains (before our next beta lands with the fix). I also confirmed that the fixes in #1373 and #1311 don't materially fix the situation of a stalled delete, either.

This PR is a real improvement to locking though, and I intend to merge it. I verified it is safe, and all relevant tests pass. And again, it more closely matches upstream while retaining the lock around the hashset. However, it's worth noting that it doesn't stop the delete from stalling and thus blocking other operations. Your app will still need to wait out the operation or determine why that is taking so long. I confirmed there is not a deadlock here, too.

I strongly suspect something is wrong with your SMB configuration, network, security scanning/protections, or underlying hardware. You may want to consider i.e. only writing to the index locally on one server, then perhaps replicating that to the readers. Or, simpler (if it works with your architecture and scaling needs), write locally and expose the local index read-only over SMB for readers, or expose an API that reads from it locally. Upstream Lucene has several warnings about running on network filesystems like NFS (which could likely be generalized to include SMB), so it's worth ensuring it's both necessary and a good fit to use SMB to solve your problem first. Hope this helps.

[marionoack]

Thank you very much for this detailed and thoughtful feedback. I can fully agree with your assessment and the conclusions you've drawn.
We are already aware that the actual challenges originate from the SMB share layer rather than Lucene itself. Addressing those underlying issues, as well as the known warnings and limitations regarding network filesystems, is something we are actively working on through a parallel internal track.
Thanks again for taking the time to investigate this so thoroughly and for confirming both the improvements of the PR and the absence of a deadlock. Your insights are very helpful and appreciated.