I haven't quite looked in to the code but based on the description in the email, probably we should use Monitor.TryEnter that way a thread enters the critical section code otherwise tells that another thread is already executing that code
Thanks & Regards On Fri, Jul 21, 2017 at 10:01 AM, Van Den Berghe, Vincent < [email protected]> wrote: > Hello everyone, > > I spent an inordinate amount of time looking at the implementation of > NativeFSLockFactory, and I see a potential problem. > > Consider the IsLocked() implementation: this method determines if a lock > is available, but only does so by trying to Obtain the lock and releasing > it, reporting false if it succeeds. > If the lock is free and two processes A and B both call IsLocked() to > determine if it is, there is a nonzero chance that only one of them will > report success, since the second process may try to obtain the lock that > the first process is temporarily holding, causing its method to return > true. Normally, both should return false if the lock is free. > > Since I'm in a complaining mood: I find the Dipose() pattern on the locks > very confusing, since the conventional implementation pattern specifies > that an IDisposable object is not to be used anymore after Dispose() is > called. However, after disposing a lock you can call Obtain() again with no > ill consequences. The cycle can be repeated ad nauseam, a fact that used in > some tests. > > Recent implementations of the native locks in java get rid of the > "Obtain", and the IsLocked (a Good Thing), so in the far far future these > problems will solve themselves. > > For now, I'd like to submit an implementation that attempts to corrects > the IsLocked problem, without giving any guarantee that it won't introduce > other problems or make existing ones go away. Sadly, the implementation > corrects them only in regular .NET. In the version of .NET Core we are > using, file locking is not implemented, and the implementation falls back > to its old behavior. > > It is much closer to the java method of doing things. Let me know what you > think and feel free to improve (and correct bugs). > > using Lucene.Net.Util; > using System; > using System.IO; > using System.Collections.Generic; > > namespace Lucene.Net.Store > { > /* > * Licensed to the Apache Software Foundation (ASF) under > one or more > * contributor license agreements. See the NOTICE file > distributed with > * this work for additional information regarding copyright > ownership. > * The ASF licenses this file to You under the Apache > License, Version 2.0 > * (the "License"); you may not use this file except in > compliance with > * the License. You may obtain a copy of the License at > * > * http://www.apache.org/licenses/LICENSE-2.0 > * > * Unless required by applicable law or agreed to in > writing, software > * distributed under the License is distributed on an "AS > IS" BASIS, > * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either > express or implied. > * See the License for the specific language governing > permissions and > * limitations under the License. > */ > > /// <summary> > /// <para>Implements <see cref="LockFactory"/> using > native OS file > /// locks. For NFS based access to an index, it's > /// recommended that you try <see > cref="SimpleFSLockFactory"/> > /// first and work around the one limitation that a lock > file > /// could be left when the runtime exits abnormally.</para> > /// > /// <para>The primary benefit of <see > cref="NativeFSLockFactory"/> is > /// that locks (not the lock file itsself) will be properly > /// removed (by the OS) if the runtime has an abnormal > exit.</para> > /// > /// <para>Note that, unlike <see > cref="SimpleFSLockFactory"/>, the existence of > /// leftover lock files in the filesystem is fine because > the OS > /// will free the locks held against these files even > though the > /// files still remain. Lucene will never actively remove > the lock > /// files, so although you see them, the index may not be > locked.</para> > /// > /// <para>Special care needs to be taken if you change the > locking > /// implementation: First be certain that no writer is in > fact > /// writing to the index otherwise you can easily corrupt > /// your index. Be sure to do the <see > cref="LockFactory"/> change on all Lucene > /// instances and clean up all leftover lock files before > starting > /// the new configuration for the first time. Different > implementations > /// can not work together!</para> > /// > /// <para>If you suspect that this or any other <see > cref="LockFactory"/> is > /// not working properly in your environment, you can > easily > /// test it by using <see cref="VerifyingLockFactory"/>, > /// <see cref="LockVerifyServer"/> and <see > cref="LockStressTest"/>.</para> > /// </summary> > /// <seealso cref="LockFactory"/> > public class NativeFSLockFactory : FSLockFactory > { > /// <summary> > /// Create a <see > cref="NativeFSLockFactory"/> instance, with <c>null</c> (unset) > /// lock directory. When you pass this > factory to a <see cref="FSDirectory"/> > /// subclass, the lock directory is > automatically set to the > /// directory itself. Be sure to create > one instance for each directory > /// your create! > /// </summary> > public NativeFSLockFactory() > : this((DirectoryInfo)null) > { > } > > /// <summary> > /// Create a <see > cref="NativeFSLockFactory"/> instance, storing lock > /// files into the specified <paramref > name="lockDirName"/> > /// </summary> > /// <param name="lockDirName"> where lock > files are created. </param> > public NativeFSLockFactory(string > lockDirName) > : this(new > DirectoryInfo(lockDirName)) > { > } > > /// <summary> > /// Create a <see > cref="NativeFSLockFactory"/> instance, storing lock > /// files into the specified <paramref > name="lockDir"/> > /// </summary> > /// <param name="lockDir"> where lock > files are created. </param> > public NativeFSLockFactory(DirectoryInfo > lockDir) > { > SetLockDir(lockDir); > } > > // LUCENENET: NativeFSLocks in Java are > infact singletons; this is how we mimick that to track instances and make > sure > // IW.Unlock and IW.IsLocked works > correctly > internal static readonly > Dictionary<string, NativeFSLock> _locks = new Dictionary<string, > NativeFSLock>(); > > /// <summary> > /// Given a lock name, return the full > prefixed path of the actual lock file. > /// </summary> > /// <param name="lockName"></param> > /// <returns></returns> > private string GetPathOfLockFile(string > lockName) > { > if (m_lockPrefix != null) > { > lockName = > m_lockPrefix + "-" + lockName; > } > return > Path.Combine(m_lockDir.FullName, lockName); > } > > public override Lock MakeLock(string > lockName) > { > var path = > GetPathOfLockFile(lockName); > NativeFSLock l; > lock(_locks) > if > (!_locks.TryGetValue(path, out l)) > > _locks.Add(path, l = new NativeFSLock(this, m_lockDir, path)); > return l; > } > > public override void ClearLock(string > lockName) > { > var path = > GetPathOfLockFile(lockName); > NativeFSLock l; > // this is the reason why > we can't use ConcurrentDictionary: we need the removal and disposal of the > lock to be atomic > // otherwise it may clash > with MakeLock making a lock and ClearLock disposing of it in another thread. > lock (_locks) > if > (_locks.TryGetValue(path, out l)) > { > > _locks.Remove(path); > > l.Dispose(); > } > } > } > > internal class NativeFSLock : Lock > { > #if NETSTANDARD > private const int ERROR_SHARE_VIOLATION = > 0x20; > #else > private const int ERROR_LOCK_VIOLATION = > 0x21; > #endif > > private readonly NativeFSLockFactory > outerInstance; > > private FileStream channel; > private readonly string path; > private readonly DirectoryInfo lockDir; > > public NativeFSLock(NativeFSLockFactory > outerInstance, DirectoryInfo lockDir, string path) > { > this.outerInstance = > outerInstance; > this.lockDir = lockDir; > this.path = path; > } > > /// <summary> > /// Return true if the <see > cref="IOException"/> is the result of a lock violation > /// </summary> > /// <param name="e"></param> > /// <returns></returns> > private static bool > IsLockOrShareViolation(IOException > e) > { > var result = e.HResult & > 0xFFFF; > #if NETSTANDARD > return result == > ERROR_SHARE_VIOLATION; > #else > return result == > ERROR_LOCK_VIOLATION; > #endif > } > > private FileStream > GetLockFileStream(FileMode mode) > { > if > (!System.IO.Directory.Exists(lockDir.FullName)) > { > try > { > > System.IO.Directory.CreateDirectory(lockDir.FullName); > } > catch > (Exception e) > { > > // note that several processes might have been trying to create the > same directory at the same time. > > // if one succeeded, the directory will exist and the exception can > be ignored. In all other cases we should report it. > > if (!System.IO.Directory.Exists(lockDir.FullName)) > > throw new IOException("Cannot create directory: " + > lockDir.FullName, e); > } > } > else if > (File.Exists(lockDir.FullName)) > { > throw new > IOException("Found regular file where directory expected: " + > lockDir.FullName); > } > > #if NETSTANDARD > return new > FileStream(path, mode, FileAccess.Write, FileShare.None, 1, mode == > FileMode.Open ? FileOptions.None : FileOptions.DeleteOnClose); > #else > return new > FileStream(path, mode, FileAccess.Write, FileShare.ReadWrite); > #endif > } > > public override bool Obtain() > { > lock (this) > { > > FailureReason = null; > > if > (channel != null) > { > > // Our instance is already locked: > > return false; > } > > #if NETSTANDARD > try > { > > channel = GetLockFileStream(FileMode.OpenOrCreate); > } > catch > (IOException e) when(IsLockOrShareViolation(e)) > { > > // no failure reason to be recorded, since this is the expected error > if a lock exists > } > catch > (Exception e) > { > > // all other exceptions need to be recorded > > FailureReason = e; > } > #else > FileStream > stream = null; > try > { > > stream = GetLockFileStream(FileMode.OpenOrCreate); > } > catch > (IOException e) > { > > FailureReason = e; > } > // > LUCENENET: UnauthorizedAccessException does not derive from IOException > like in java > catch > (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; > } > > if (stream > != null) > > try > > { > > stream.Lock(0, 1); > > // only assign the channel if the lock succeeds > > channel = stream; > > } > > catch (Exception e) > > { > > FailureReason = e; > > IOUtils.DisposeWhileHandlingException(stream); > > } > #endif > return > channel != null; > } > } > > protected override void Dispose(bool > disposing) > { > if (disposing) > { > lock (this) > { > > if (channel != null) > > { > > try > > { > > NativeFSLockFactory._locks. > Remove(path); > > } > > finally > > { > > IOUtils. > DisposeWhileHandlingException(channel); > > channel = null; > > } > #if !NETSTANDARD > > // try to delete the file if we created it, but it's > not an error if we can't. > > try > > { > > File.Delete(path); > > } > > catch > > { > > } > #endif > > } > } > } > } > > public override bool IsLocked() > { > lock (this) > { > // First a > shortcut, if a lock reference in this instance is available > if > (channel != null) > { > > return true; > } > > #if NETSTANDARD > try > { > > using (var stream = GetLockFileStream(FileMode.Open)) > > { > > } > > return false; > } > catch > (IOException e) when (IsLockOrShareViolation(e)) > { > > return true; > } > catch > (FileNotFoundException) > { > > // if the file doesn't exists, there can be no lock active > > return false; > } > #else > try > { > > using (var stream = GetLockFileStream(FileMode.Open)) > > { > > // try to find out if the file is locked by writing a > byte. Note that we need to flush the stream to find out. > > stream.WriteByte(0); > > stream.Flush(); // this *may* throw an IOException > if the file is locked, but... > > > // ... closing the stream is the > real test > > } > > return false; > } > catch > (IOException e) when (IsLockOrShareViolation(e)) > { > > return true; > } > catch > (FileNotFoundException) > { > > // if the file doesn't exists, there can be no lock active > > return false; > } > #endif > } > } > > public override string ToString() > { > return "NativeFSLock@" + > path; > } > } > } > > >
