BUG: Lucene.Net.Store.NRTCachingDirectory: Don't throw exceptions whe…

…n Dispose() is called multiple times. Also added checks to ensure an ObjectDisposedException is thrown by any method call after Dispose() is called. Fixed all other directory implementations and made the check atomic so only the first call to Dispose() is run. Fixes apache#841. Related to apache#265.

src/Lucene.Net.TestFramework/Store/BaseDirectoryTestCase.cs

@@ -1,6 +1,7 @@
 // Lucene version compatibility level 8.2.0
 // LUCENENET NOTE: This class now exists both here and in Lucene.Net.Tests
 using J2N.Threading;
+using Lucene.Net.Attributes;
 using Lucene.Net.Index;
 using Lucene.Net.Support;
 using Lucene.Net.Util;
@@ -455,6 +456,18 @@ public virtual void TestDetectClose()
             });
         }
 
+        /// <summary>
+        /// Make sure directory allows double-dispose as per the
+        /// <a href="https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern">dispose pattern docs</a>.
+        /// </summary>
+        [Test]
+        [LuceneNetSpecific] // GH-841, GH-265
+        public virtual void TestDoubleDispose()
+        {
+            using Directory dir = GetDirectory(CreateTempDir("testDoubleDispose"));
+            Assert.DoesNotThrow(() => dir.Dispose());
+        }
+
         //        private class ListAllThread : ThreadJob
         //        {
         //            private readonly BaseDirectoryTestCase outerInstance;

src/Lucene.Net.TestFramework/Store/BaseDirectoryWrapper.cs

@@ -1,5 +1,6 @@
-using Lucene.Net.Index;
+using Lucene.Net.Index;
 using Lucene.Net.Util;
+using System.Threading;
 
 namespace Lucene.Net.Store
 {
@@ -30,7 +31,48 @@ public class BaseDirectoryWrapper : FilterDirectory
     {
         private bool checkIndexOnClose = true;
         private bool crossCheckTermVectorsOnClose = true;
-        private volatile bool isOpen = true; // LUCENENET specific - private because volatile is not CLS compliant, but made protected setter
+
+        // LUCENENET specific - setup to make it safe to call dispose multiple times
+        private const int True = 1;
+        private const int False = 0;
+
+        // LUCENENET specific - using Interlocked intead of a volatile field for IsOpen.
+        private int isOpen = True; // LUCENENET: Changed from bool to int so we can use Interlocked.
+
+        /// <summary>
+        /// Atomically sets the value to the given updated value
+        /// if the current value <c>==</c> the expected value.
+        /// <para/>
+        /// Expert: Use this in the <see cref="Dispose(bool)"/> call to skip
+        /// duplicate calls by using the folling if block to guard the
+        /// dispose logic.
+        /// <code>
+        /// protected override void Dispose(bool disposing)
+        /// {
+        ///     if (!CompareAndSetIsOpen(expect: true, update: false)) return;
+        /// 
+        ///     // Dispose unmanaged resources
+        ///     if (disposing)
+        ///     {
+        ///         // Dispose managed resources
+        ///     }
+        /// }
+        /// </code>
+        /// </summary>
+        /// <param name="expect">The expected value (the comparand).</param>
+        /// <param name="update">The new value.</param>
+        /// <returns><c>true</c> if successful. A <c>false</c> return value indicates that
+        /// the actual value was not equal to the expected value.</returns>
+        // LUCENENET specific - setup to make it safe to call dispose multiple times
+        protected internal bool CompareAndSetIsOpen(bool expect, bool update)
+        {
+            int e = expect ? True : False;
+            int u = update ? True : False;
+
+            int original = Interlocked.CompareExchange(ref isOpen, u, e);
+
+            return original == e;
+        }
 
         public BaseDirectoryWrapper(Directory @delegate)
             : base(@delegate)
@@ -39,9 +81,11 @@ public BaseDirectoryWrapper(Directory @delegate)
 
         protected override void Dispose(bool disposing)
         {
+            if (!CompareAndSetIsOpen(expect: true, update: false)) return; // LUCENENET: allow dispose more than once as per https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern
+
             if (disposing)
             {
-                isOpen = false;
+                // LUCENENET: Removed setter for isOpen and put it above in the if check so it is atomic
                 if (checkIndexOnClose && DirectoryReader.IndexExists(this))
                 {
                     TestUtil.CheckIndex(this, crossCheckTermVectorsOnClose);
@@ -50,10 +94,15 @@ protected override void Dispose(bool disposing)
             }
         }
 
+        /// <summary>
+        /// Gets a value indicating whether the current <see cref="Directory"/> instance is open.
+        /// <para/>
+        /// Expert: This is useful for implementing the <see cref="EnsureOpen()"/> logic.
+        /// </summary>
         public virtual bool IsOpen
         {
-            get => isOpen;
-            protected set => isOpen = value; // LUCENENET specific - added protected setter and marked isOpen private because volatile is not CLS compliant
+            get => Interlocked.CompareExchange(ref isOpen, False, False) == True ? true : false;
+            protected set => Interlocked.Exchange(ref this.isOpen, value ? True : False); // LUCENENET specific - added protected setter
         }
 
         /// <summary>

src/Lucene.Net.TestFramework/Store/MockDirectoryWrapper.cs

@@ -919,6 +919,8 @@ public virtual bool WrapLockFactory
 
         protected override void Dispose(bool disposing)
         {
+            if (!CompareAndSetIsOpen(expect: true, update: false)) return; // LUCENENET: allow dispose more than once as per https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern
+
             UninterruptableMonitor.Enter(this);
             try
             {
@@ -1097,8 +1099,16 @@ protected override void Dispose(bool disposing)
                             }
                         }
                     }
-                    m_input.Dispose(); // LUCENENET TODO: using blocks in this entire class
-                    throttledOutput.Dispose(); // LUCENENET specific
+                    // LUCENENET specific: While the Microsoft docs say that Dispose() should not throw errors,
+                    // we are being defensive because this is a test mock.
+                    try
+                    {
+                        m_input.Dispose();
+                    }
+                    finally
+                    {
+                        throttledOutput.Dispose(); // LUCENENET specific
+                    }
                 }
             }
             finally

src/Lucene.Net.Tests.TestFramework/Support/TestApiConsistency.cs

@@ -38,7 +38,7 @@ public override void TestProtectedFieldNames(Type typeFromTargetAssembly)
         [TestCase(typeof(Lucene.Net.RandomExtensions))]
         public override void TestPrivateFieldNames(Type typeFromTargetAssembly)
         {
-            base.TestPrivateFieldNames(typeFromTargetAssembly, @"ApiScanTestBase|TestUtil\.MaxRecursionBound|Assert\.FailureFormat|Lucene\.Net\.Util\.RandomizedContext\.RandomizedContextPropertyName|Lucene\.Net\.Util\.DefaultNamespaceTypeWrapper\.AllMembers");
+            base.TestPrivateFieldNames(typeFromTargetAssembly, @"ApiScanTestBase|TestUtil\.MaxRecursionBound|Assert\.FailureFormat|Lucene\.Net\.Util\.RandomizedContext\.RandomizedContextPropertyName|Lucene\.Net\.Util\.DefaultNamespaceTypeWrapper\.AllMembers|^Lucene\.Net\.Store\.BaseDirectoryWrapper\.(?:True|False)");
         }
 
         [Test, LuceneNetSpecific]

src/Lucene.Net.Tests/Store/TestDirectory.cs

@@ -57,6 +57,20 @@ public virtual void TestDetectClose()
             }
         }
 
+        [Test]
+        [LuceneNetSpecific] // GH-841, GH-265
+        public virtual void TestDoubleDispose()
+        {
+            DirectoryInfo tempDir = CreateTempDir(GetType().Name);
+            Directory[] dirs = new Directory[] { new RAMDirectory(), new SimpleFSDirectory(tempDir), new NIOFSDirectory(tempDir) };
+
+            foreach (Directory dir in dirs)
+            {
+                Assert.DoesNotThrow(() => dir.Dispose());
+                Assert.DoesNotThrow(() => dir.Dispose());
+            }
+        }
+
         // test is occasionally very slow, i dont know why
         // try this seed: 7D7E036AD12927F5:93333EF9E6DE44DE
         [Test]

src/Lucene.Net.Tests/Store/TestNRTCachingDirectory.cs

@@ -5,6 +5,7 @@
 using JCG = J2N.Collections.Generic;
 using Assert = Lucene.Net.TestFramework.Assert;
 using Console = Lucene.Net.Util.SystemConsole;
+using Lucene.Net.Attributes;
 
 namespace Lucene.Net.Store
 {
@@ -203,6 +204,18 @@ public virtual void TestCompoundFileAppendTwice()
             newDir.Dispose();
         }
 
+        /// <summary>
+        /// Make sure directory allows double-dispose as per the
+        /// <a href="https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern">dispose pattern docs</a>.
+        /// </summary>
+        [Test]
+        [LuceneNetSpecific]
+        public virtual void TestDoubleDispose()
+        {
+            using Directory newDir = new NRTCachingDirectory(NewDirectory(), 2.0, 25.0);
+            Assert.DoesNotThrow(() => newDir.Dispose());
+        }
+
         /// <summary>
         /// Creates a file of the specified size with sequential data. The first
         ///  byte is written as the start byte provided. All subsequent bytes are

src/Lucene.Net.Tests/Support/TestApiConsistency.cs

@@ -38,7 +38,7 @@ public override void TestProtectedFieldNames(Type typeFromTargetAssembly)
         [TestCase(typeof(Lucene.Net.Analysis.Analyzer))]
         public override void TestPrivateFieldNames(Type typeFromTargetAssembly)
         {
-            base.TestPrivateFieldNames(typeFromTargetAssembly, @"^Lucene\.Net\.Support\.(?:ConcurrentHashSet|PlatformHelper|DateTimeOffsetUtil|Arrays|IO\.FileSupport)|^Lucene\.ExceptionExtensions|^Lucene\.Net\.Util\.Constants\.MaxStackByteLimit|^Lucene\.Net\.Search\.TopDocs\.ShardByteSize");
+            base.TestPrivateFieldNames(typeFromTargetAssembly, @"^Lucene\.Net\.Support\.(?:ConcurrentHashSet|PlatformHelper|DateTimeOffsetUtil|Arrays|IO\.FileSupport)|^Lucene\.ExceptionExtensions|^Lucene\.Net\.Util\.Constants\.MaxStackByteLimit|^Lucene\.Net\.Search\.TopDocs\.ShardByteSize|^Lucene\.Net\.Store\.BaseDirectory\.(?:True|False)");
         }
 
         [Test, LuceneNetSpecific]

src/Lucene.Net/Store/BaseDirectory.cs

@@ -1,5 +1,6 @@
 using Lucene.Net.Diagnostics;
 using System;
+using System.Threading;
 
 namespace Lucene.Net.Store
 {
@@ -27,15 +28,57 @@ namespace Lucene.Net.Store
     /// </summary>
     public abstract class BaseDirectory : Directory
     {
-        private volatile bool isOpen = true;
+        // LUCENENET specific - setup to make it safe to call dispose multiple times
+        private const int True = 1;
+        private const int False = 0;
 
-        // LUCENENET specific - since we can't make a CLS-compliant 
-        // protected volatile field, we are exposing it through a protected
-        // property.
+        // LUCENENET specific - using Interlocked intead of a volatile field for IsOpen.
+        private int isOpen = True; // LUCENENET: Added check to ensure we aren't disposed.
+
+        /// <summary>
+        /// Gets a value indicating whether the current <see cref="Directory"/> instance is open.
+        /// <para/>
+        /// Expert: This is useful for implementing the <see cref="EnsureOpen()"/> logic.
+        /// </summary>
         protected internal virtual bool IsOpen
         {
-            get => isOpen;
-            set => isOpen = value;
+            get => Interlocked.CompareExchange(ref isOpen, False, False) == True ? true : false;
+            set => Interlocked.Exchange(ref this.isOpen, value ? True : False);
+        }
+
+        /// <summary>
+        /// Atomically sets the value to the given updated value
+        /// if the current value <c>==</c> the expected value.
+        /// <para/>
+        /// Expert: Use this in the <see cref="Dispose(bool)"/> call to skip
+        /// duplicate calls by using the folling if block to guard the
+        /// dispose logic.
+        /// <code>
+        /// protected override void Dispose(bool disposing)
+        /// {
+        ///     if (!CompareAndSetIsOpen(expect: true, update: false)) return;
+        /// 
+        ///     // Dispose unmanaged resources
+        ///     if (disposing)
+        ///     {
+        ///         // Dispose managed resources
+        ///     }
+        /// }
+        /// </code>
+        /// </summary>
+        /// <param name="expect">The expected value (the comparand).</param>
+        /// <param name="update">The new value.</param>
+        /// <returns><c>true</c> if successful. A <c>false</c> return value indicates that
+        /// the actual value was not equal to the expected value.</returns>
+        // LUCENENET specific - setup to make it safe to call dispose multiple times
+        protected internal bool CompareAndSetIsOpen(bool expect, bool update)
+        {
+            int e = expect ? True : False;
+            int u = update ? True : False;
+
+            int original = Interlocked.CompareExchange(ref isOpen, u, e);
+
+            return original == e;
         }
 
         /// <summary>

src/Lucene.Net/Store/CompoundFileDirectory.cs

@@ -286,17 +286,15 @@ private static IDictionary<string, FileEntry> ReadLegacyEntries(IndexInput strea
 
         protected override void Dispose(bool disposing)
         {
+            // allow double close - usually to be consistent with other closeables
+            if (!CompareAndSetIsOpen(expect: true, update: false)) return; // already closed
+
             UninterruptableMonitor.Enter(this);
             try
             {
                 if (disposing)
                 {
-                    if (!IsOpen)
-                    {
-                        // allow double close - usually to be consistent with other closeables
-                        return; // already closed
-                    }
-                    IsOpen = false;
+                    // LUCENENET: Moved double-dispose logic outside of lock.
                     if (writer != null)
                     {
                         if (Debugging.AssertsEnabled) Debugging.Assert(openForWrite);
@@ -455,6 +453,7 @@ public IndexInputSlicerAnonymousClass(CompoundFileDirectory outerInstance, FileE
 
             protected override void Dispose(bool disposing)
             {
+                // LUCENENET: Intentionally blank
             }
 
             public override IndexInput OpenSlice(string sliceDescription, long offset, long length)

src/Lucene.Net/Store/FSDirectory.cs

@@ -423,10 +423,7 @@ public override string GetLockID()
         /// Closes the store to future operations. </summary>
         protected override void Dispose(bool disposing)
         {
-            if (disposing)
-            {
-                IsOpen = false;
-            }
+            IsOpen = false; // LUCENENET: Since there is nothing else to do here, we can safely call this. If we have other stuff to dispose, change to if (!CompareAndSetIsOpen(expect: true, update: false)) return;
         }
 
         /// <summary> the underlying filesystem directory </summary>