From 8d977135c228c8322609ee9209b9b7f235a384bc Mon Sep 17 00:00:00 2001 From: Paul Irwin Date: Wed, 15 May 2024 20:19:17 -0600 Subject: [PATCH] Synchronize access to stale files collection This is necessary to prevent race conditions, even though this code is not in the upstream Java code. A thread could try to add an item to the collection after it has been synced in `Sync` but before it is removed from the collection, then the file is removed from the collection, resulting in a missed sync. --- src/Lucene.Net/Store/FSDirectory.cs | 108 ++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 31 deletions(-) diff --git a/src/Lucene.Net/Store/FSDirectory.cs b/src/Lucene.Net/Store/FSDirectory.cs index 3eaa3f123d..63d92f9cca 100644 --- a/src/Lucene.Net/Store/FSDirectory.cs +++ b/src/Lucene.Net/Store/FSDirectory.cs @@ -1,5 +1,7 @@ using Lucene.Net.Support; using Lucene.Net.Support.IO; +using Lucene.Net.Support.Threading; +using Lucene.Net.Util; using System; using System.Collections.Generic; using System.Globalization; @@ -29,9 +31,6 @@ namespace Lucene.Net.Store * limitations under the License. */ - using Constants = Lucene.Net.Util.Constants; - using IOUtils = Lucene.Net.Util.IOUtils; - /// /// Base class for implementations that store index /// files in the file system. @@ -80,7 +79,7 @@ namespace Lucene.Net.Store /// in .NET /// in conjunction with an open because it is not guaranteed to exit atomically. /// Any lock statement or call can throw a - /// , which makes shutting down unpredictable. + /// , which makes shutting down unpredictable. /// To exit parallel tasks safely, we recommend using s /// and "interrupt" them with s. /// @@ -94,7 +93,24 @@ public abstract class FSDirectory : BaseDirectory public const int DEFAULT_READ_CHUNK_SIZE = 8192; protected readonly DirectoryInfo m_directory; // The underlying filesystem directory - protected readonly ISet m_staleFiles = new ConcurrentHashSet(); // Files written, but not yet sync'ed + + /// + /// The collection of stale files that need to be 'ed + /// + /// + /// LUCENENET NOTE: This is a non-thread-safe collection so that we can synchronize access to it + /// using the field. This is to prevent race conditions, i.e. one thread + /// adding a file to the collection while another thread is trying to sync the files, which could + /// cause a missed sync. If you need to access this collection from a derived type, you should + /// synchronize access to it using the protected field. + /// + protected readonly ISet m_staleFiles = new HashSet(); // Files written, but not yet sync'ed + + /// + /// A object to synchronize access to the collection. + /// You should synchronize access to using this object from derived types. + /// + protected readonly object syncLock = new object(); #pragma warning disable 612, 618 private int chunkSize = DEFAULT_READ_CHUNK_SIZE; @@ -299,28 +315,39 @@ public override long FileLength(string name) public override void DeleteFile(string name) { EnsureOpen(); - FileInfo file = new FileInfo(Path.Combine(m_directory.FullName, name)); - // LUCENENET specific: We need to explicitly throw when the file has already been deleted, - // since FileInfo doesn't do that for us. - // (An enhancement carried over from Lucene 8.2.0) - if (!File.Exists(file.FullName)) - { - throw new FileNotFoundException("Cannot delete " + file + " because it doesn't exist."); - } + string file = Path.Combine(m_directory.FullName, name); + + // LUCENENET Specific: See remarks for m_staleFiles field. + UninterruptableMonitor.Enter(syncLock); try { - file.Delete(); - if (File.Exists(file.FullName)) + // LUCENENET specific: We need to explicitly throw when the file has already been deleted, + // since FileInfo doesn't do that for us. + // (An enhancement carried over from Lucene 8.2.0) + if (!File.Exists(file)) + { + throw new FileNotFoundException("Cannot delete " + file + " because it doesn't exist."); + } + + try + { + File.Delete(file); + if (File.Exists(file)) + { + throw new IOException("Cannot delete " + file); + } + } + catch (Exception e) { - throw new IOException("Cannot delete " + file); + throw new IOException("Cannot delete " + file, e); } + + m_staleFiles.Remove(name); } - catch (Exception e) + finally { - throw new IOException("Cannot delete " + file, e); + UninterruptableMonitor.Exit(syncLock); } - - m_staleFiles.Remove(name); } /// @@ -363,7 +390,16 @@ protected virtual void EnsureCanWrite(string name) protected virtual void OnIndexOutputClosed(FSIndexOutput io) { - m_staleFiles.Add(io.name); + // LUCENENET Specific: See remarks for m_staleFiles field. + UninterruptableMonitor.Enter(syncLock); + try + { + m_staleFiles.Add(io.name); + } + finally + { + UninterruptableMonitor.Exit(syncLock); + } } public override void Sync(ICollection names) @@ -371,21 +407,31 @@ public override void Sync(ICollection names) EnsureOpen(); ISet toSync = new HashSet(names); - toSync.IntersectWith(m_staleFiles); - foreach (var name in toSync) + // LUCENENET Specific: See remarks for m_staleFiles field. + UninterruptableMonitor.Enter(syncLock); + try { - Fsync(name); - } + toSync.IntersectWith(m_staleFiles); - // fsync the directory itself, but only if there was any file fsynced before - // (otherwise it can happen that the directory does not yet exist)! - if (toSync.Count > 0) + foreach (var name in toSync) + { + Fsync(name); + } + + // fsync the directory itself, but only if there was any file fsynced before + // (otherwise it can happen that the directory does not yet exist)! + if (toSync.Count > 0) + { + IOUtils.Fsync(m_directory.FullName, true); + } + + m_staleFiles.ExceptWith(toSync); + } + finally { - IOUtils.Fsync(m_directory.FullName, true); + UninterruptableMonitor.Exit(syncLock); } - - m_staleFiles.ExceptWith(toSync); } public override string GetLockID()