Fix file locking problem on Chorus Notes (LT-20807)

* In https://jira.sil.org/browse/LT-20807 the problem seems to come up
  when multiple processes are trying to write the file

+semver:minor

src/LibChorus/notes/AnnotationMutexFactory.cs

@@ -0,0 +1,63 @@
+using System.Threading;
+using SIL.PlatformUtilities;
+
+namespace Chorus.notes
+{
+	/// <summary>
+	/// This factory class creates a cross process mutex for locking Annotation (ChorusNotes) write operations
+	/// </summary>
+	internal class AnnotationMutexFactory
+	{
+		public static IAnnotationMutex Create(string annotationFilePath)
+		{
+			var mutexName = $"ChorusAnnotationRepositoryMutex_{annotationFilePath.GetHashCode()}";
+			if (Platform.IsWindows)
+			{
+				return new WinMutex(false, mutexName);
+			}
+			return new LinuxMutex(mutexName);
+		}
+
+		/// <summary>
+		/// This class will create a named mutex which locks across processes
+		/// </summary>
+		private class WinMutex : IAnnotationMutex
+		{
+			private readonly Mutex _mutex;
+
+			public WinMutex(bool initiallyOwned, string mutexName)
+			{
+				_mutex = new Mutex(initiallyOwned, mutexName);
+			}
+
+			public void WaitOne()
+			{
+				_mutex.WaitOne();
+			}
+
+			public void ReleaseMutex()
+			{
+				_mutex.ReleaseMutex();
+			}
+		}
+
+		/// <summary>
+		/// Currently just a no-op interface implementation since named mutexes are not implemented in Mono
+		/// Enhance: implement a platform appropriate cross process file locking mechanism
+		/// </summary>
+		private class LinuxMutex: IAnnotationMutex
+		{
+			public LinuxMutex(string mutexName)
+			{
+			}
+
+			public void WaitOne()
+			{
+			}
+
+			public void ReleaseMutex()
+			{
+			}
+		}
+	}
+}

src/LibChorus/notes/AnnotationRepository.cs

@@ -15,22 +15,18 @@ namespace Chorus.notes
 	public class AnnotationRepository : IDisposable, IAnnotationRepository
 	{
 		private XDocument _doc;
-		private readonly string _annotationFilePath;
 		private DateTime _lastAnnotationFileWriteTime;
 		private static int kCurrentVersion=0;
 		public static string FileExtension = "ChorusNotes";
 		private List<IAnnotationRepositoryObserver> _observers = new List<IAnnotationRepositoryObserver>();
 		private AnnotationIndex _indexOfAllAnnotationsByKey;
 		private bool _isDirty;
-		private FileSystemWatcher _watcher = null;
-		private bool _writingFileOurselves = false;
+		private FileSystemWatcher _watcher;
 
-		public string AnnotationFilePath
-		{
-			get { return _annotationFilePath; }
-		}
+		public string AnnotationFilePath { get; }
+		public IAnnotationMutex SavingMutex { get; set; }
 
-		public static AnnotationRepository FromFile(string primaryRefParameter, string path, IProgress progress)
+	  public static AnnotationRepository FromFile(string primaryRefParameter, string path, IProgress progress)
 		{
 			try
 			{
@@ -39,19 +35,24 @@ public static AnnotationRepository FromFile(string primaryRefParameter, string p
 					RequireThat.Directory(Path.GetDirectoryName(path)).Exists();
 					File.WriteAllText(path, string.Format("<notes version='{0}'/>", kCurrentVersion.ToString()));
 				}
-				var doc = XDocument.Load(path);
-				ThrowIfVersionTooHigh(doc, path);
-				var result = new AnnotationRepository(primaryRefParameter, doc, path, progress);
-				// Set up a watcher so that if something else...typically FLEx...modifies our file, we update our display.
-				// This is useful in its own right for keeping things consistent, but more importantly, if we don't do
-				// this and then the user does something in FlexBridge that changes the file, we could overwrite the
-				// changes made elsewhere (LT-20074).
-				result.UpateAnnotationFileWriteTime();
-				result._watcher = new FileSystemWatcher(Path.GetDirectoryName(path), Path.GetFileName(path));
-				result._watcher.NotifyFilter = NotifyFilters.LastWrite;
-				result._watcher.Changed += result.UnderlyingFileChanged;
-				result._watcher.EnableRaisingEvents = true;
-				return result;
+
+				using (var reader = new FileStream(path, FileMode.Open, FileAccess.Read,
+					FileShare.ReadWrite))
+				{
+					var doc = XDocument.Load(reader);
+					ThrowIfVersionTooHigh(doc, path);
+					var result = new AnnotationRepository(primaryRefParameter, doc, path, progress);
+					// Set up a watcher so that if something else...typically FLEx...modifies our file, we update our display.
+					// This is useful in its own right for keeping things consistent, but more importantly, if we don't do
+					// this and then the user does something in FlexBridge that changes the file, we could overwrite the
+					// changes made elsewhere (LT-20074).
+					result.UpateAnnotationFileWriteTime();
+					result._watcher = new FileSystemWatcher(Path.GetDirectoryName(path), Path.GetFileName(path));
+					result._watcher.NotifyFilter = NotifyFilters.LastWrite;
+					result._watcher.Changed += result.UnderlyingFileChanged;
+					result._watcher.EnableRaisingEvents = true;
+					return result;
+				}
 			}
 			catch (XmlException error)
 			{
@@ -73,12 +74,6 @@ private void UpateAnnotationFileWriteTime()
 		/// <param name="e"></param>
 		private void UnderlyingFileChanged(object sender, FileSystemEventArgs e)
 		{
-			if (_writingFileOurselves)
-			{
-				// Sometimes a file changed event fires after we wrote the file but before we noted the new modify time.
-				// In that case there is nothing we need to do here.
-				return;
-			}
 			var fileOnDiskWriteTime = _lastAnnotationFileWriteTime;
 			var fileAccessible = false;
 			try
@@ -118,10 +113,21 @@ private void UnderlyingFileChanged(object sender, FileSystemEventArgs e)
 			try
 			{
 				UpateAnnotationFileWriteTime();
-				_doc = XDocument.Load(AnnotationFilePath);
+				using (var reader = new FileStream(AnnotationFilePath, FileMode.Open,
+					FileAccess.Read, FileShare.ReadWrite))
+				{
+					_doc = XDocument.Load(reader);
+				}
+
 				SetupDocument();
 				_observers.ForEach(observer => observer.NotifyOfStaleList());
 			}
+			catch (XmlException)
+			{
+				// If someone is writing to the file and it is incomplete we expect to get a new notification.
+				// Roll back the write time so that we will try again.
+				_lastAnnotationFileWriteTime = writeTimeBeforeLoad;
+			}
 			catch (IOException)
 			{
 				// If someone is writing to the file and blocking access, hopefully we will get an new notification.
@@ -147,7 +153,8 @@ public static AnnotationRepository FromString(string primaryRefParameter, string
 		public AnnotationRepository(string primaryRefParameter, XDocument doc, string path, IProgress progress)
 		{
 			_doc = doc;
-			_annotationFilePath = path;
+			AnnotationFilePath = path;
+			SavingMutex = AnnotationMutexFactory.Create(AnnotationFilePath);
 
 			SetupDocument();
 			SetPrimaryRefParameter(primaryRefParameter, progress);
@@ -193,10 +200,7 @@ public void Dispose()
 				}
 			}
 
-			if (_watcher != null)
-			{
-				_watcher.Dispose();
-			}
+			_watcher?.Dispose();
 			SaveNowIfNeeded(new NullProgress());
 			_doc = null;
 		}
@@ -234,26 +238,30 @@ where Annotation.GetStatusOfLastMessage(a) == status
 				   select new Annotation(a);
 		}
 
-//        public void SaveAs(string path)
-//        {
-//            _doc.Save(path);
-//        }
-
 		public void Save(IProgress progress)
 		{
+			SavingMutex.WaitOne();
 			try
 			{
+				// _watcher can be null in unit tests
+				if (_watcher != null)
+				{
+					// We have a lock on the file so only we can be changing it
+					_watcher.Changed -= UnderlyingFileChanged;
+				}
+
 				if (string.IsNullOrEmpty(AnnotationFilePath))
 				{
 					throw new InvalidOperationException(
 						"Cannot save if the repository was created from a string");
 				}
 
 				progress.WriteStatus("Saving Chorus Notes...");
-				_writingFileOurselves = true;
-				using (var writer = XmlWriter.Create(AnnotationFilePath,
+
+				using (var writeStream = new FileStream(AnnotationFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite))
+				using (var writer = XmlWriter.Create(writeStream,
 					CanonicalXmlSettings.CreateXmlWriterSettings())
-					)
+				)
 				{
 					_doc.Save(writer);
 				}
@@ -262,14 +270,21 @@ public void Save(IProgress progress)
 				_isDirty = false;
 				UpateAnnotationFileWriteTime();
 			}
-			catch(Exception e)
+			catch (Exception e)
 			{
-				SIL.Reporting.ErrorReport.NotifyUserOfProblem(e, "Chorus has a problem saving notes for {0}.",
-																 _annotationFilePath);
+				SIL.Reporting.ErrorReport.NotifyUserOfProblem(e,
+					"Chorus has a problem saving notes for {0}.",
+					AnnotationFilePath);
 			}
 			finally
 			{
-				_writingFileOurselves = false;
+				SavingMutex.ReleaseMutex();
+				// _watcher can be null in unit tests
+				if (_watcher != null)
+				{
+					// Now that the notifications for the file change have fired we will listen again
+					_watcher.Changed += UnderlyingFileChanged;
+				}
 			}
 		}
 
@@ -374,7 +389,7 @@ private static IEnumerable<string> GetChorusNotesFilePaths(string path)
 
 		public bool ContainsAnnotation(Annotation annotation)
 		{
-			return null!= _doc.Root.Elements().FirstOrDefault(e => e.GetAttributeValue("guid") == annotation.Guid);
+			return null != _doc.Root.Elements().FirstOrDefault(e => e.GetAttributeValue("guid") == annotation.Guid);
 		}
 	}
 

src/LibChorus/notes/IAnnotationMutex.cs

@@ -0,0 +1,11 @@
+namespace Chorus.notes
+{
+	/// <summary>
+	/// This interface wraps a named mutex for locking annotation files so we can have platform specific implementations
+	/// </summary>
+	public interface IAnnotationMutex
+	{
+		void WaitOne();
+		void ReleaseMutex();
+	}
+}

src/LibChorus/sync/Synchronizer.cs

@@ -11,6 +11,7 @@
 using Chorus.VcsDrivers.Mercurial;
 using System.Linq;
 using Chorus.Model;
+using Chorus.notes;
 using SIL.Progress;
 using SIL.Reporting;
 using SIL.Xml;
@@ -746,11 +747,25 @@ private static void AppendAnyNewNotes(string localRepositoryPath)
 						var newOldNode = oldDoc.ImportNode(node, true);
 						oldNotesNode.AppendChild(newOldNode);
 					}
-					using (var fileWriter = XmlWriter.Create(oldNotesFile, CanonicalXmlSettings.CreateXmlWriterSettings()))
+
+					var mutex = AnnotationMutexFactory.Create(oldNotesFile);
+					try
+					{
+						mutex.WaitOne();
+						using (var fileStream = new FileStream(oldNotesFile, FileMode.Open,
+							FileAccess.ReadWrite, FileShare.Read))
+						using (var fileWriter = XmlWriter.Create(fileStream,
+							CanonicalXmlSettings.CreateXmlWriterSettings()))
+						{
+							oldDoc.Save(fileWriter);
+						}
+
+						File.Delete(newNote);
+					}
+					finally
 					{
-						oldDoc.Save(fileWriter);
+						mutex.ReleaseMutex();
 					}
-					File.Delete(newNote);
 				}
 				else
 				{
@@ -762,15 +777,13 @@ private static void AppendAnyNewNotes(string localRepositoryPath)
 
 		private bool CheckAndWarnIfNoCommonAncestor(Revision a, Revision b )
 		{
-			if (null ==Repository.GetCommonAncestorOfRevisions(a.Number.Hash,b.Number.Hash))
-			{
-				_progress.WriteWarning(
-					"This repository has an anomaly:  the two heads we want to merge have no common ancestor.  You should get help from the developers of this application.");
-				_progress.WriteWarning("1) \"{0}\" on {1} by {2} ({3}). ", a.GetHashCode(), a.Summary, a.DateString, a.UserId);
-				_progress.WriteWarning("2) \"{0}\" on {1} by {2} ({3}). ", b.GetHashCode(), b.Summary, b.DateString, b.UserId);
-				return true;
-			}
-			return false;
+			if (Repository.GetCommonAncestorOfRevisions(a.Number.Hash, b.Number.Hash) != null)
+				return false;
+			_progress.WriteWarning(
+				"This repository has an anomaly:  the two heads we want to merge have no common ancestor.  You should get help from the developers of this application.");
+			_progress.WriteWarning("1) \"{0}\" on {1} by {2} ({3}). ", a.GetHashCode(), a.Summary, a.DateString, a.UserId);
+			_progress.WriteWarning("2) \"{0}\" on {1} by {2} ({3}). ", b.GetHashCode(), b.Summary, b.DateString, b.UserId);
+			return true;
 		}
 
 		/// <returns>false if nothing needed to be merged, true if the merge was done. Throws exception if there is an error.</returns>