Permalink
Browse files

[Ide] #1303 - Exception from recent files when opening file

When adding items to the recent files list, the RecentFileStorageClass
was performing removal of the old item and insertion of the new item
as separate operations. The removal would trigger other instances of MD
to read the changed file, and there was a high chance that their lock
on the file would prevent the addition. The class would retry reads
but not writes. The net effect was that items would vanish from the list.

This is fixed by performing all read/remove/add/write operations with
a single file handle.

NOTE: this was only an issue on Windows, which implies either that the
FSW or the file locking were not working reliably on Mac/Linux, or
simply that the race didn't happen due to differences in speed of
operations.
  • Loading branch information...
1 parent a9a0dfc commit 4865068db5d67a595efa91cb287d8b438f36b821 @mhutch mhutch committed Mar 22, 2012
@@ -54,7 +54,7 @@ internal sealed class RecentFileStorage : IDisposable
string filePath;
FileSystemWatcher watcher;
- object writerLock = new object ();
+ List<RecentItem> cachedItemList;
public static string DefaultPath {
get {
@@ -97,175 +97,179 @@ void DisableWatching ()
void FileChanged (object sender, FileSystemEventArgs e)
{
- OnRecentFilesChanged (EventArgs.Empty);
+ OnRecentFilesChanged (null);
}
void HandleWatcherRenamed (object sender, RenamedEventArgs e)
{
- OnRecentFilesChanged (EventArgs.Empty);
- }
+ OnRecentFilesChanged (null);
+ }
- bool FilterOut (Func<RecentItem,bool> pred)
+ public bool RemoveItem (string uri)
{
- lock (writerLock) {
- bool filteredSomething = false;
- List<RecentItem> store = ReadStore (0);
- if (store != null) {
- for (int i = 0; i < store.Count; ++i) {
- if (pred (store[i])) {
- store.RemoveAt (i);
- filteredSomething = true;
- --i;
- continue;
- }
- }
- if (filteredSomething)
- WriteStore (store);
- }
- return filteredSomething;
- }
+ return ModifyStore (list => RemoveMatches (list, item => item.Uri != null && item.Uri.Equals (uri)));
}
- ///operation should return true if it modified an item
- bool RunOperation (Func<RecentItem,bool> operation)
+ public bool RemoveItem (RecentItem item)
{
- lock (writerLock) {
- bool changedSomething = false;
- List<RecentItem> store = ReadStore (0);
- if (store != null) {
- for (int i = 0; i < store.Count; ++i)
- changedSomething |= operation (store[i]);
- if (changedSomething)
- WriteStore (store);
- }
- return changedSomething;
- }
+ return item != null && RemoveItem (item.Uri);
}
- public void ClearGroup (params string[] groups)
+ public bool RenameItem (string oldUri, string newUri)
{
- FilterOut (item => groups.Any (g => item.IsInGroup (g)));
+ if (oldUri == null || newUri == null)
+ return false;
+
+ return ModifyStore (list => {
+ bool modified = false;
+ foreach (var item in list) {
+ if (item.Uri == oldUri) {
+ string oldName = Path.GetFileName (item.LocalPath);
+ item.Uri = newUri;
+ if (item.Private.Contains (oldName)) {
+ item.Private = item.Private.Replace (oldName, Path.GetFileName (item.LocalPath));
+ }
+ item.NewTimeStamp ();
+ modified = true;
+ }
+ }
+ return modified;
+ });
}
- public void RemoveMissingFiles (params string[] groups)
+ public RecentItem[] GetItemsInGroup (string group)
{
- FilterOut (item => item.IsFile && groups.Any (g => item.IsInGroup (g)) && !File.Exists (item.LocalPath));
+ var c = cachedItemList;
+ if (c == null) {
+ List<RecentItem> list;
+ using (var fs = AcquireFileExclusive (filePath)) {
+ c = cachedItemList = ReadStore (fs);
+ }
+ }
+ c.Sort ();
+ return c.Where (item => item.IsInGroup (group)).ToArray ();
}
- public bool RemoveItem (string uri)
+ public void RemoveMissingFiles (params string[] groups)
{
- return uri != null && FilterOut (item => item.Uri != null && item.Uri.Equals (uri));
+ ModifyStore (list => RemoveMatches (list, item =>
+ item.IsFile && groups.Any (g => item.IsInGroup (g)) && !File.Exists (item.LocalPath)
+ ));
}
- public bool RemoveItem (RecentItem item)
+ public void ClearGroup (params string[] groups)
{
- return item != null && RemoveItem (item.Uri);
+ ModifyStore (list => RemoveMatches (list, item => groups.Any (group => item.IsInGroup (group))));
}
- public bool RenameItem (string oldUri, string newUri)
+ public void AddWithLimit (RecentItem item, string group, int limit)
{
- if (oldUri == null || newUri == null)
- return false;
- return RunOperation (delegate(RecentItem item) {
- if (item.Uri == null)
- return false;
- if (item.Uri == oldUri) {
- string oldName = Path.GetFileName (item.LocalPath);
- item.Uri = newUri;
- if (item.Private.Contains (oldName)) {
- item.Private = item.Private.Replace (oldName, Path.GetFileName (item.LocalPath));
- }
- item.NewTimeStamp ();
- return true;
- }
- return false;
+ ModifyStore (list => {
+ RemoveMatches (list, i => i.Uri == item.Uri);
+ list.Add (item);
+ CheckLimit (list, group, limit);
+ return true;
});
}
- public RecentItem[] GetItemsInGroup (string group)
+ static bool CheckLimit (List<RecentItem> list, string group, int limit)
{
- List<RecentItem> result = new List<RecentItem> ();
- RunOperation (delegate(RecentItem item) {
- if (item.IsInGroup (group))
- result.Add (item);
- return false;
- });
- result.Sort ();
- return result.ToArray ();
+ list.Sort ();
+ bool modified = false;
+ int count = 0;
+ for (int i = 0; i < list.Count; i++) {
+ if (list[i].IsInGroup (group) && (++count > limit)) {
+ list.RemoveAt (i);
+ i--;
+ }
+ }
+ return modified;
}
- void CheckLimit (string group, int limit)
+ static bool RemoveMatches<T> (List<T> list, Func<T,bool> predicate)
{
- RecentItem[] items = GetItemsInGroup (group);
- for (int i = limit; i < items.Length; i++)
- this.RemoveItem (items[i]);
+ bool modified = false;
+ for (int i = list.Count - 1; i >= 0; i--) {
+ if (predicate (list[i])) {
+ list.RemoveAt (i);
+ modified = true;
+ }
+ }
+ return modified;
}
- public void AddWithLimit (RecentItem item, string group, int limit)
+ bool ModifyStore (Func<List<RecentItem>,bool> modify)
{
- lock (writerLock) {
- RemoveItem (item.Uri);
- List<RecentItem> store = ReadStore (0);
- if (store != null) {
- store.Add (item);
- WriteStore (store);
- CheckLimit (group, limit);
+ List<RecentItem> list;
+ using (var fs = AcquireFileExclusive (filePath)) {
+ list = ReadStore (fs);
+ if (!modify (list)) {
+ return false;
}
+ fs.Position = 0;
+ fs.SetLength (0);
+ WriteStore (fs, list);
}
+ //TODO: can we suppress duplicate event from the FSW?
+ OnRecentFilesChanged (list);
+ return true;
}
- const int MAX_TRIES = 5;
- List<RecentItem> ReadStore (int numberOfTry)
+
+ static List<RecentItem> ReadStore (FileStream file)
{
- List<RecentItem> result = new List<RecentItem> ();
- if (!File.Exists (filePath))
+ var result = new List<RecentItem> ();
+ if (file.Length == 0) {
return result;
- var reader = new XmlTextReader (filePath);
- try {
- while (true) {
- bool read = false;
- try {
- // seems to crash on empty files ?
- read = reader.Read ();
- } catch (Exception) {
- read = false;
- }
- if (!read)
- break;
- if (reader.IsStartElement () && reader.LocalName == RecentItem.Node)
+ }
+
+ using (var reader = XmlReader.Create (file, new XmlReaderSettings { CloseInput = false })) {
+ while (reader.Read ()) {
+ if (reader.IsStartElement () && reader.LocalName == RecentItem.Node) {
result.Add (RecentItem.Read (reader));
+ }
}
- } catch (Exception e) {
- MonoDevelop.Core.LoggingService.LogError ("Exception while reading the store", e);
- if (numberOfTry < MAX_TRIES) {
- Thread.Sleep (200);
- return ReadStore (numberOfTry + 1);
- }
- return null;
- } finally {
- if (reader != null)
- reader.Close ();
}
+
return result;
}
static Encoding utf8WithoutByteOrderMark = new UTF8Encoding (false);
- void WriteStore (List<RecentItem> items)
+
+ static void WriteStore (FileStream stream, List<RecentItem> items)
{
items.Sort ();
if (items.Count > MaxRecentItemsCount)
items.RemoveRange (MaxRecentItemsCount, items.Count - MaxRecentItemsCount);
- var writer = new XmlTextWriter (filePath, utf8WithoutByteOrderMark);
- try {
+ using (var writer = new XmlTextWriter (stream, utf8WithoutByteOrderMark)) {
writer.Formatting = Formatting.Indented;
writer.WriteStartDocument ();
writer.WriteStartElement ("RecentFiles");
if (items != null)
foreach (RecentItem item in items)
item.Write (writer);
writer.WriteEndElement (); // RecentFiles
- } finally {
- writer.Close ();
- OnRecentFilesChanged (EventArgs.Empty);
+ }
+ }
+
+ //FIXME: should we P/Invoke lockf on POSIX or is Mono's FileShare.None sufficient?
+ static FileStream AcquireFileExclusive (string filePath)
+ {
+ const int MAX_WAIT_TIME = 1000;
+ const int RETRY_WAIT = 50;
+
+ int remainingTries = MAX_WAIT_TIME / RETRY_WAIT;
+ while (true) {
+ try {
+ return File.Open (filePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None);
+ } catch (Exception ex) {
+ //FIXME: will it work on Mono if we check that it's an access conflict, i.e. HResult is 0x80070020?
+ if (ex is IOException && remainingTries > 0) {
+ Thread.Sleep (RETRY_WAIT);
+ remainingTries--;
+ continue;
+ }
+ throw;
+ }
}
}
@@ -274,10 +278,12 @@ public static string ToUri (string fileName)
return fileName.StartsWith ("file://") ? fileName : "file://" + fileName;
}
- void OnRecentFilesChanged (EventArgs e)
+ void OnRecentFilesChanged (List<RecentItem> cachedItemList)
{
- if (changed != null)
- changed (this, e);
+ this.cachedItemList = cachedItemList;
+ if (changed != null) {
+ changed (this, EventArgs.Empty);
+ }
}
EventHandler changed;
@@ -304,4 +310,4 @@ public void Dispose ()
DisableWatching ();
}
}
-}
+}
@@ -271,4 +271,4 @@ public void Write (XmlWriter writer)
}
#endregion
}
-}
+}

0 comments on commit 4865068

Please sign in to comment.