From dev-return-10423-apmail-lucenenet-dev-archive=lucenenet.apache.org@lucenenet.apache.org Mon Jul 24 11:41:10 2017 Return-Path: X-Original-To: apmail-lucenenet-dev-archive@www.apache.org Delivered-To: apmail-lucenenet-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id AFFB61A2FE for ; Mon, 24 Jul 2017 11:41:10 +0000 (UTC) Received: (qmail 5334 invoked by uid 500); 24 Jul 2017 11:41:05 -0000 Delivered-To: apmail-lucenenet-dev-archive@lucenenet.apache.org Received: (qmail 5298 invoked by uid 500); 24 Jul 2017 11:41:05 -0000 Mailing-List: contact dev-help@lucenenet.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@lucenenet.apache.org Delivered-To: mailing list dev@lucenenet.apache.org Received: (qmail 5286 invoked by uid 99); 24 Jul 2017 11:41:05 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 24 Jul 2017 11:41:05 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id AE85A1806B0 for ; Mon, 24 Jul 2017 11:41:04 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=0 tagged_above=-999 required=6.31 tests=[none] autolearn=disabled Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id 0UqLFshNiuMs for ; Mon, 24 Jul 2017 11:41:01 +0000 (UTC) Received: from ex10cshbfe01.apps4rent.net (ex10cshbfe01.apps4rent.net [69.160.246.194]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with SMTP id 8F05B5FB84 for ; Mon, 24 Jul 2017 11:41:00 +0000 (UTC) Received: from EX10DAG10-N1.apps4rent.net ([10.10.10.123]) by EX10CSHBFE01 ([10.10.10.113]) with mapi id 14.03.0319.002; Mon, 24 Jul 2017 04:40:53 -0700 From: Shad Storhaug To: "Van Den Berghe, Vincent" CC: "dev@lucenenet.apache.org" Subject: RE: An alternative NativeFSLockFactory Thread-Topic: An alternative NativeFSLockFactory Thread-Index: AdMCK1HLH4GvTURgS6ObACIjDItMoAAIPiYwAIeCIRA= Date: Mon, 24 Jul 2017 11:41:08 +0000 Message-ID: <458A3CD4F362D144B999930AAADEBAAD14B6E19A@EX10DAG10-N1> References: <458A3CD4F362D144B999930AAADEBAAD14B6D77F@EX10DAG10-N1> In-Reply-To: <458A3CD4F362D144B999930AAADEBAAD14B6D77F@EX10DAG10-N1> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [1.20.184.29] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Vincent, I took a look and indeed FileStream.Lock is coming back in .NET Standard 2.= 0 (https://apisof.net/catalog/System.IO.FileStream.Lock(Int64,Int64)), so I= have created a FEATURE_FILESTREAM_LOCK so we can put it back in when the t= ime comes. Also, I noticed that in the Obtain method under the NETSTANDARD symbol, you= have: try { channel =3D 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 =3D e; } In the original (https://github.com/apache/lucenenet/blob/c3f60b29f54ac1c5c= c2d3e94f17a27208c13683c/src/Lucene.Net/Store/NativeFSLockFactory.cs#L149-L1= 76), the code does not swallow all exceptions, but catches specific excepti= ons. Is this intentional or a bug? Note that Lucene's design often depends on specific exceptions being thrown= to go down the right execution path, so I just wanted to be sure we aren't= shooting ourselves in the foot by catching everything here. BTW - Jens has submitted the Replicator PR (https://github.com/apache/lucen= enet/pull/209). I would appreciate any feedback you may have. Thanks, Shad Storhaug (NightOwl888) -----Original Message----- From: Shad Storhaug [mailto:shad@shadstorhaug.com]=20 Sent: Saturday, July 22, 2017 1:58 AM To: Van Den Berghe, Vincent Cc: dev@lucenenet.apache.org Subject: RE: An alternative NativeFSLockFactory Vincent, Thanks for doing the legwork.=20 Tests To make sure we can maintain the same functionality through future porting = efforts, we should have a test or two that fail without this patched lockin= g (at least part of the time) and succeed with the fix in place. Maybe thos= e tests will just need to be skipped on .NET Core (at least for now), but a= t least going forward we can detect this issue if repeated again. Features If you take a look and it turns out file locking is coming back in .NET Sta= ndard 2.0 (you can drill into the "2.0" link here https://docs.microsoft.co= m/en-us/dotnet/standard/net-standard to see a diff of the APIs that are bei= ng added), we should change this to FEATURE_FILE_LOCKING instead of NETSTAN= DARD so we can put this feature back in when we support .NET Standard 2.0. You can look at https://github.com/apache/lucenenet/pull/191 for a list of = features that we support (it may be out of date a little), which we can hop= efully add back in for .NET Standard 2.0. Thanks, Shad Storhaug (NightOwl888) -----Original Message----- From: Van Den Berghe, Vincent [mailto:Vincent.VanDenBerghe@bvdinfo.com]=20 Sent: Friday, July 21, 2017 10:02 PM To: dev@lucenenet.apache.org Subject: An alternative NativeFSLockFactory Hello everyone, I spent an inordinate amount of time looking at the implementation of Nativ= eFSLockFactory, and I see a potential problem. Consider the IsLocked() implementation: this method determines if a lock i= s 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 deter= mine 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. Normall= y, both should return false if the lock is free. Since I'm in a complaining mood: I find the Dipose() pattern on the locks v= ery 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 co= nsequences. 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 w= ill 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 oth= er problems or make existing ones go away. Sadly, the implementation correc= ts them only in regular .NET. In the version of .NET Core we are using, fil= e 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 on= e or more * contributor license agreements. See the NOTICE file dist= ributed with * this work for additional information regarding copyright = ownership. * The ASF licenses this file to You under the Apache Licens= e, Version 2.0 * (the "License"); you may not use this file except in comp= liance 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 expr= ess or implied. * See the License for the specific language governing permi= ssions and * limitations under the License. */ /// /// Implements using nati= ve OS file /// locks. For NFS based access to an index, it's /// recommended that you try /// first and work around the one limitation that a lock fi= le /// could be left when the runtime exits abnormally. /// /// The primary benefit of is /// that locks (not the lock file itsself) will be properly /// removed (by the OS) if the runtime has an abnormal exit= . /// /// Note that, unlike , the existence of /// leftover lock files in the filesystem is fine because t= he OS /// will free the locks held against these files even thoug= h the /// files still remain. Lucene will never actively remove t= he lock /// files, so although you see them, the index may not be l= ocked. /// /// Special care needs to be taken if you change the = locking /// implementation: First be certain that no writer is in f= act /// writing to the index otherwise you can easily corrupt /// your index. Be sure to do the change on all Lucene /// instances and clean up all leftover lock files before s= tarting /// the new configuration for the first time. Different imp= lementations /// can not work together! /// /// If you suspect that this or any other is /// not working properly in your environment, you can easil= y /// test it by using , /// and . /// /// public class NativeFSLockFactory : FSLockFactory { /// /// Create a instance, with null (unset) /// lock directory. When you pass this fact= ory to a /// subclass, the lock directory is automat= ically set to the /// directory itself. Be sure to create one= instance for each directory /// your create! /// public NativeFSLockFactory() : this((DirectoryInfo)null) { } /// /// Create a instance, storing lock /// files into the specified /// /// where lock= files are created. public NativeFSLockFactory(string lockDirNa= me) : this(new DirectoryInfo(lo= ckDirName)) { } /// /// Create a instance, storing lock /// files into the specified /// /// where lock fil= es are created. public NativeFSLockFactory(DirectoryInfo lo= ckDir) { SetLockDir(lockDir); } // LUCENENET: NativeFSLocks in Java are inf= act singletons; this is how we mimick that to track instances and make sure // IW.Unlock and IW.IsLocked works correctl= y internal static readonly Dictionary _locks =3D new Dictionary(); /// /// Given a lock name, return the full pref= ixed path of the actual lock file. /// /// /// private string GetPathOfLockFile(string loc= kName) { if (m_lockPrefix !=3D null) { lockName = =3D m_lockPrefix + "-" + lockName; } return Path.Combine(m_lockD= ir.FullName, lockName); } public override Lock MakeLock(string lockNa= me) { var path =3D GetPathOfLockF= ile(lockName); NativeFSLock l; lock(_locks) if (!_locks= .TryGetValue(path, out l)) = _locks.Add(path, l =3D new NativeFSLock(this, m_lockDir, path)); return l; } public override void ClearLock(string lockN= ame) { var path =3D GetPathOfLockF= ile(lockName); NativeFSLock l; // this is the reason why w= e can't use ConcurrentDictionary: we need the removal and disposal of the l= ock to be atomic // otherwise it may clash w= ith 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 =3D= 0x20; #else private const int ERROR_LOCK_VIOLATION =3D = 0x21; #endif private readonly NativeFSLockFactory outerI= nstance; private FileStream channel; private readonly string path; private readonly DirectoryInfo lockDir; public NativeFSLock(NativeFSLockFactory out= erInstance, DirectoryInfo lockDir, string path) { this.outerInstance =3D oute= rInstance; this.lockDir =3D lockDir; this.path =3D path; } /// /// Return true if the is the result of a lock violation /// /// /// private static bool IsLockOrShareViolation(= IOException e) { var result =3D e.HResult & = 0xFFFF; #if NETSTANDARD return result =3D=3D ERROR_= SHARE_VIOLATION; #else return result =3D=3D ERROR_= LOCK_VIOLATION; #endif } private FileStream GetLockFileStream(FileMo= de mode) { if (!System.IO.Directory.Ex= ists(lockDir.FullName)) { try { = System.IO.Directory.CreateDirectory(lockDir.FullName); } catch (Exce= ption e) { = // note that several processes might have been trying to create the sa= me 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: " + lo= ckDir.FullName, e); } } else if (File.Exists(lockDi= r.FullName)) { throw new I= OException("Found regular file where directory expected: " + lockDir.FullNa= me); } #if NETSTANDARD return new FileStream(path,= mode, FileAccess.Write, FileShare.None, 1, mode =3D=3D FileMode.Open ? Fil= eOptions.None : FileOptions.DeleteOnClose); #else return new FileStream(path,= mode, FileAccess.Write, FileShare.ReadWrite); #endif } public override bool Obtain() { lock (this) { FailureReas= on =3D null; if (channel= !=3D null) { = // Our instance is already locked: = return false; } #if NETSTANDARD try { = channel =3D GetLockFileStream(FileMode.OpenOrCreate); } catch (IOEx= ception e) when(IsLockOrShareViolation(e)) { = // no failure reason to be recorded, since this is the expected error = if a lock exists } catch (Exce= ption e) { = // all other exceptions need to be recorded = FailureReason =3D e; } #else FileStream = stream =3D null; try { = stream =3D GetLockFileStream(FileMode.OpenOrCreate); } catch (IOEx= ception e) { = FailureReason =3D e; } // LUCENENE= T: UnauthorizedAccessException does not derive from IOException like in jav= a catch (Unau= thorizedAccessException 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 =3D e; } if (stream = !=3D null) = try = { = stream.Lock(0, 1); = // only assign the channel if the lock succeeds = channel =3D stream; = } = catch (Exception e) = { = FailureReason =3D e; = IOUtils.DisposeWhileHandlingException(stream); = } #endif return chan= nel !=3D null; } } protected override void Dispose(bool dispos= ing) { if (disposing) { lock (this) { = if (channel !=3D null) = { = try = { = NativeFSLockFactory._locks.Remove(path= ); = } = finally = { = IOUtils.DisposeWhileHandlingException(= channel); = channel =3D null; = } #if !NETSTANDARD = // try to delete the file if we created it, but it's n= ot 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= !=3D null) { = return true; } #if NETSTANDARD try { = using (var stream =3D GetLockFileStream(FileMode.Open)) = { = } = return false; } catch (IOEx= ception e) when (IsLockOrShareViolation(e)) { = return true; } catch (File= NotFoundException) { = // if the file doesn't exists, there can be no lock active = return false; } #else try { = using (var stream =3D 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 i= f the file is locked, but... = = // ... closing the stream is the = real test = } = return false; } catch (IOEx= ception e) when (IsLockOrShareViolation(e)) { = return true; } catch (File= NotFoundException) { = // if the file doesn't exists, there can be no lock active = return false; } #endif } } public override string ToString() { return "NativeFSLock@" + pa= th; } } }