lucenenet-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From synhers...@apache.org
Subject [09/18] lucenenet git commit: Fix NativeFSLock, round 1
Date Tue, 06 Jan 2015 03:46:25 GMT
Fix NativeFSLock, round 1

We should be making locks properly based on the characteristics of the CLR and Windows

All relevant unit tests are now green


Project: http://git-wip-us.apache.org/repos/asf/lucenenet/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucenenet/commit/8a999e1f
Tree: http://git-wip-us.apache.org/repos/asf/lucenenet/tree/8a999e1f
Diff: http://git-wip-us.apache.org/repos/asf/lucenenet/diff/8a999e1f

Branch: refs/heads/master
Commit: 8a999e1f6629ffa16d51bf7ebef8f303d5c7ca3f
Parents: cb2dfdc
Author: Itamar Syn-Hershko <itamar@code972.com>
Authored: Mon Jan 5 17:47:48 2015 +0200
Committer: Itamar Syn-Hershko <itamar@code972.com>
Committed: Mon Jan 5 17:47:48 2015 +0200

----------------------------------------------------------------------
 src/Lucene.Net.Core/Store/Lock.cs               |   7 +-
 .../Store/NativeFSLockFactory.cs                | 246 ++++---------------
 .../Util/LuceneTestCase.cs                      |   1 -
 .../core/Store/TestLockFactory.cs               |  27 +-
 4 files changed, 62 insertions(+), 219 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/lucenenet/blob/8a999e1f/src/Lucene.Net.Core/Store/Lock.cs
----------------------------------------------------------------------
diff --git a/src/Lucene.Net.Core/Store/Lock.cs b/src/Lucene.Net.Core/Store/Lock.cs
index a0682c8..722d9fc 100644
--- a/src/Lucene.Net.Core/Store/Lock.cs
+++ b/src/Lucene.Net.Core/Store/Lock.cs
@@ -32,9 +32,7 @@ namespace Lucene.Net.Store
     ///   }.run();
     /// </pre>
     /// </summary>
-    /// <seealso cref= Directory#makeLock(String)
-    ///
-    /// @lucene.internal </seealso>
+    /// <seealso cref="Directory#makeLock(String)"/>
     public abstract class Lock : IDisposable
     {
         /// <summary>
@@ -61,7 +59,7 @@ namespace Lucene.Net.Store
         /// with the "root cause" Exception as to why the lock was
         /// not obtained.
         /// </summary>
-        protected internal Exception FailureReason;
+        public Exception FailureReason { get; protected set; }
 
         /// <summary>
         /// Attempts to obtain an exclusive lock within amount of
@@ -121,6 +119,7 @@ namespace Lucene.Net.Store
 
         public virtual void Dispose()
         {
+            Release();
         }
 
         /// <summary>

http://git-wip-us.apache.org/repos/asf/lucenenet/blob/8a999e1f/src/Lucene.Net.Core/Store/NativeFSLockFactory.cs
----------------------------------------------------------------------
diff --git a/src/Lucene.Net.Core/Store/NativeFSLockFactory.cs b/src/Lucene.Net.Core/Store/NativeFSLockFactory.cs
index e21210a..fd98b94 100644
--- a/src/Lucene.Net.Core/Store/NativeFSLockFactory.cs
+++ b/src/Lucene.Net.Core/Store/NativeFSLockFactory.cs
@@ -1,6 +1,9 @@
+using System;
+using System.Collections.Concurrent;
+using Lucene.Net.Util;
+
 namespace Lucene.Net.Store
 {
-    using System.Collections.Generic;
     using System.IO;
 
     /*
@@ -112,13 +115,11 @@ namespace Lucene.Net.Store
 
     internal class NativeFSLock : Lock
     {
-        private System.IO.FileStream f;
         private FileStream Channel;
-        private bool @lock;
-        private DirectoryInfo Path;
-        private DirectoryInfo LockDir;
+        private readonly DirectoryInfo Path;
+        private readonly DirectoryInfo LockDir;
 
-        /*
+      /*
        * The javadocs for FileChannel state that you should have
        * a single instance of a FileChannel (per JVM) for all
        * locking against a given file.  To ensure this, we have
@@ -128,7 +129,7 @@ namespace Lucene.Net.Store
        * one JVM (each with their own NativeFSLockFactory
        * instance) have set the same lock dir and lock prefix.
        */
-        private static HashSet<string> LOCK_HELD = new HashSet<string>();
+        private static readonly ConcurrentDictionary<string, WeakReference<FileStream>>
LOCKS_HELD = new ConcurrentDictionary<string, WeakReference<FileStream>>();
 
         public NativeFSLock(DirectoryInfo lockDir, string lockFileName)
         {
@@ -136,32 +137,19 @@ namespace Lucene.Net.Store
             Path = new DirectoryInfo(System.IO.Path.Combine(lockDir.FullName, lockFileName));
         }
 
-        private bool LockExists()
-        {
-            lock (this)
-            {
-                return @lock != false;
-            }
-        }
-
         public override bool Obtain()
         {
             lock (this)
             {
-                if (LockExists())
+                if (Channel != null)
                 {
                     // Our instance is already locked:
                     return false;
                 }
 
-                // Ensure that lockDir exists and is a directory.
-                bool tmpBool;
-                if (System.IO.File.Exists(LockDir.FullName))
-                    tmpBool = true;
-                else
-                    tmpBool = System.IO.Directory.Exists(LockDir.FullName);
+                //LOCKS_HELD.GetOrAdd(Path.FullName)
 
-                if (!tmpBool)
+                if (!System.IO.Directory.Exists(LockDir.FullName))
                 {
                     try
                     {
@@ -172,192 +160,57 @@ namespace Lucene.Net.Store
                         throw new System.IO.IOException("Cannot create directory: " + LockDir.FullName);
                     }
                 }
-                else if (!System.IO.Directory.Exists(LockDir.FullName))
+                else if (System.IO.File.Exists(LockDir.FullName))
                 {
                     throw new System.IO.IOException("Found regular file where directory expected:
" + LockDir.FullName);
                 }
 
-                string canonicalPath = Path.FullName;
-
-                bool markedHeld = false;
-
+                bool success = false;
                 try
                 {
-                    // Make sure nobody else in-process has this lock held
-                    // already, and, mark it held if not:
-
-                    lock (LOCK_HELD)
-                    {
-                        if (LOCK_HELD.Contains(canonicalPath))
-                        {
-                            // Someone else in this JVM already has the lock:
-                            return false;
-                        }
-                        else
-                        {
-                            // This "reserves" the fact that we are the one
-                            // thread trying to obtain this lock, so we own
-                            // the only instance of a channel against this
-                            // file:
-                            LOCK_HELD.Add(canonicalPath);
-                            markedHeld = true;
-                        }
-                    }
-
-                    try
-                    {
-                        f = new System.IO.FileStream(Path.FullName, System.IO.FileMode.Create,
System.IO.FileAccess.Write);
-                    }
-                    catch (System.IO.IOException e)
-                    {
-                        // On Windows, we can get intermittent "Access
-                        // Denied" here.  So, we treat this as failure to
-                        // acquire the lock, but, store the reason in case
-                        // there is in fact a real error case.
-                        FailureReason = e;
-                        f = null;
-                    }
-                    // lucene.net: UnauthorizedAccessException does not derive from IOException
like in java
-                    catch (System.UnauthorizedAccessException e)
-                    {
-                        // On Windows, we can get intermittent "Access
-                        // Denied" here.  So, we treat this as failure to
-                        // acquire the lock, but, store the reason in case
-                        // there is in fact a real error case.
-                        FailureReason = e;
-                        f = null;
-                    }
-
-                    if (f != null)
+                    Channel = new FileStream(Path.FullName, FileMode.Create, FileAccess.Write,
FileShare.None);
+                    Channel.Lock(0, Channel.Length);
+                    success = true;
+                }
+                catch (IOException e)
+                {
+                    FailureReason = e;
+                    Channel = null;
+                }
+                // LUCENENET: UnauthorizedAccessException does not derive from IOException
like in java
+                catch (System.UnauthorizedAccessException e)
+                {
+                    // On Windows, we can get intermittent "Access
+                    // Denied" here.  So, we treat this as failure to
+                    // acquire the lock, but, store the reason in case
+                    // there is in fact a real error case.
+                    FailureReason = e;
+                    Channel = null;
+                }
+                finally
+                {
+                    if (!success)
                     {
                         try
                         {
-                            Channel = f;
-                            @lock = false;
-                            try
-                            {
-                                Channel.Lock(0, Channel.Length);
-                                @lock = true;
-                            }
-                            catch (System.IO.IOException e)
-                            {
-                                // At least on OS X, we will sometimes get an
-                                // intermittent "Permission Denied" IOException,
-                                // which seems to simply mean "you failed to get
-                                // the lock".  But other IOExceptions could be
-                                // "permanent" (eg, locking is not supported via
-                                // the filesystem).  So, we record the failure
-                                // reason here; the timeout obtain (usually the
-                                // one calling us) will use this as "root cause"
-                                // if it fails to get the lock.
-                                FailureReason = e;
-                            }
-                            // lucene.net: UnauthorizedAccessException does not derive from
IOException like in java
-                            catch (System.UnauthorizedAccessException e)
-                            {
-                                // At least on OS X, we will sometimes get an
-                                // intermittent "Permission Denied" IOException,
-                                // which seems to simply mean "you failed to get
-                                // the lock".  But other IOExceptions could be
-                                // "permanent" (eg, locking is not supported via
-                                // the filesystem).  So, we record the failure
-                                // reason here; the timeout obtain (usually the
-                                // one calling us) will use this as "root cause"
-                                // if it fails to get the lock.
-                                FailureReason = e;
-                            }
-                            finally
-                            {
-                                if (@lock == false)
-                                {
-                                    try
-                                    {
-                                        Channel.Close();
-                                    }
-                                    finally
-                                    {
-                                        Channel = null;
-                                    }
-                                }
-                            }
+                            IOUtils.CloseWhileHandlingException(Channel);
                         }
                         finally
                         {
-                            if (Channel == null)
-                            {
-                                try
-                                {
-                                    f.Close();
-                                }
-                                finally
-                                {
-                                    f = null;
-                                }
-                            }
-                        }
-                    }
-                }
-                finally
-                {
-                    if (markedHeld && !LockExists())
-                    {
-                        lock (LOCK_HELD)
-                        {
-                            if (LOCK_HELD.Contains(canonicalPath))
-                            {
-                                LOCK_HELD.Remove(canonicalPath);
-                            }
+                            Channel = null;
                         }
                     }
                 }
-                return LockExists();
-            }
-
-            /*
 
-              Channel = FileChannel.open(Path.toPath(), StandardOpenOption.CREATE, StandardOpenOption.WRITE);
-              bool success = false;
-              try
-              {
-                @lock = Channel.TryLock();
-                success = @lock != null;
-              }
-              catch (System.IO.IOException | OverlappingFileLockException e)
-              {
-                // At least on OS X, we will sometimes get an
-                // intermittent "Permission Denied" System.IO.IOException,
-                // which seems to simply mean "you failed to get
-                // the lock".  But other System.IO.IOExceptions could be
-                // "permanent" (eg, locking is not supported via
-                // the filesystem).  So, we record the failure
-                // reason here; the timeout obtain (usually the
-                // one calling us) will use this as "root cause"
-                // if it fails to get the lock.
-                FailureReason = e;
-              }
-              finally
-              {
-                if (!success)
-                {
-                  try
-                  {
-                    IOUtils.CloseWhileHandlingException(Channel);
-                  }
-                  finally
-                  {
-                    Channel = null;
-                  }
-                }
-              }
-              return @lock != null;
-            }*/
+                return Channel != null;
+            }
         }
 
         public override void Release()
         {
             lock (this)
             {
-                if (LockExists())
+                if (Channel != null)
                 {
                     try
                     {
@@ -365,7 +218,6 @@ namespace Lucene.Net.Store
                     }
                     finally
                     {
-                        @lock = false;
                         try
                         {
                             Channel.Close();
@@ -373,18 +225,10 @@ namespace Lucene.Net.Store
                         finally
                         {
                             Channel = null;
-                            try
-                            {
-                                f.Close();
-                            }
-                            finally
-                            {
-                                f = null;
-                                lock (LOCK_HELD)
-                                {
-                                    LOCK_HELD.Remove(Path.FullName);
-                                }
-                            }
+//                            lock (LOCK_HELD)
+//                            {
+//                                LOCK_HELD.Remove(Path.FullName);
+//                            }
                         }
                     }
                     bool tmpBool;
@@ -419,7 +263,7 @@ namespace Lucene.Net.Store
                     // The test for is isLocked is not directly possible with native file
locks:
 
                     // First a shortcut, if a lock reference in this instance is available
-                    if (LockExists())
+                    if (Channel != null)
                     {
                         return true;
                     }

http://git-wip-us.apache.org/repos/asf/lucenenet/blob/8a999e1f/src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
----------------------------------------------------------------------
diff --git a/src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs b/src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
index 9d829ea..66bf0ca 100644
--- a/src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
+++ b/src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
@@ -638,7 +638,6 @@ namespace Lucene.Net.Util
         {
             get
             {
-                //return ClassNameRule.TestClass;
                 return typeof(LuceneTestCase);
             }
         }

http://git-wip-us.apache.org/repos/asf/lucenenet/blob/8a999e1f/src/Lucene.Net.Tests/core/Store/TestLockFactory.cs
----------------------------------------------------------------------
diff --git a/src/Lucene.Net.Tests/core/Store/TestLockFactory.cs b/src/Lucene.Net.Tests/core/Store/TestLockFactory.cs
index d779d39..b74a4c7 100644
--- a/src/Lucene.Net.Tests/core/Store/TestLockFactory.cs
+++ b/src/Lucene.Net.Tests/core/Store/TestLockFactory.cs
@@ -197,22 +197,21 @@ namespace Lucene.Net.Store
         [Test]
         public virtual void TestNativeFSLockFactory()
         {
-            //NativeFSLockFactory f = new NativeFSLockFactory(CreateTempDir(LuceneTestCase.TestClass.SimpleName));
-            NativeFSLockFactory f = new NativeFSLockFactory(AppSettings.Get("tempDir", Path.GetTempPath()));
+            var f = new NativeFSLockFactory(CreateTempDir("testNativeFsLockFactory"));
 
             f.LockPrefix = "test";
-            Lock l = f.MakeLock("commit");
-            Lock l2 = f.MakeLock("commit");
+            var l = f.MakeLock("commit");
+            var l2 = f.MakeLock("commit");
 
-            Assert.IsTrue(l.Obtain(), "failed to obtain lock");
+            Assert.IsTrue(l.Obtain(), "failed to obtain lock, got exception: {0}", l.FailureReason);
             Assert.IsTrue(!l2.Obtain(), "succeeded in obtaining lock twice");
             l.Dispose();
 
-            Assert.IsTrue(l2.Obtain(), "failed to obtain 2nd lock after first one was freed");
+            Assert.IsTrue(l2.Obtain(), "failed to obtain 2nd lock after first one was freed,
got exception: {0}", l2.FailureReason);
             l2.Dispose();
 
             // Make sure we can obtain first one again, test isLocked():
-            Assert.IsTrue(l.Obtain(), "failed to obtain lock");
+            Assert.IsTrue(l.Obtain(), "failed to obtain lock, got exception: {0}", l.FailureReason);
             Assert.IsTrue(l.Locked);
             Assert.IsTrue(l2.Locked);
             l.Dispose();
@@ -224,14 +223,16 @@ namespace Lucene.Net.Store
         [Test]
         public virtual void TestNativeFSLockFactoryLockExists()
         {
-            DirectoryInfo tempDir = new DirectoryInfo(AppSettings.Get("tempDir", Path.GetTempPath()));
-            FileInfo lockFile = new FileInfo(/*tempDir, */"test.lock");
-            lockFile.Create();
+            DirectoryInfo tempDir = CreateTempDir("testNativeFsLockFactory");
 
-            Lock l = (new NativeFSLockFactory(tempDir)).MakeLock("test.lock");
-            Assert.IsTrue(l.Obtain(), "failed to obtain lock");
+            // Touch the lock file
+            var lockFile = new FileInfo(Path.Combine(tempDir.FullName, "test.lock"));
+            using (lockFile.Create()){};
+
+            var l = (new NativeFSLockFactory(tempDir)).MakeLock("test.lock");
+            Assert.IsTrue(l.Obtain(), "failed to obtain lock, got exception: {0}", l.FailureReason);
             l.Dispose();
-            Assert.IsFalse(l.Locked, "failed to release lock");
+            Assert.IsFalse(l.Locked, "failed to release lock, got exception: {0}", l.FailureReason);
             if (lockFile.Exists)
             {
                 lockFile.Delete();


Mime
View raw message