From c19a83f1d19c21b3751ada747f0808be38676ead Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 17 Jan 2014 20:07:01 +0100 Subject: [PATCH] Fix premature disposal of IProgressMonitor in parallel background search. This was the cause of "InvalidOperationException: The LinkedList node does not belong to current LinkedList." in ProgressCollector.UnregisterNamedMonitor(). --- .../Project/Engine/SearchManager.cs | 19 +++++++++++++++++-- .../Editor/Search/ISearchResultFactory.cs | 10 ++++++++++ .../Project/Editor/Search/SearchResultsPad.cs | 18 ++++++++++++++++++ .../Base/Project/Util/IProgressMonitor.cs | 2 +- .../Base/Project/Util/ProgressCollector.cs | 3 +++ 5 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/AddIns/Misc/SearchAndReplace/Project/Engine/SearchManager.cs b/src/AddIns/Misc/SearchAndReplace/Project/Engine/SearchManager.cs index 70d46c3f1f..4fc18f7df3 100644 --- a/src/AddIns/Misc/SearchAndReplace/Project/Engine/SearchManager.cs +++ b/src/AddIns/Misc/SearchAndReplace/Project/Engine/SearchManager.cs @@ -28,6 +28,14 @@ namespace SearchAndReplace public class SearchManager { #region FindAll + /// + /// Prepares a parallel search operation. + /// + /// The search strategy. Contains the search term and options. + /// The location to search in. + /// The progress monitor used to report progress. + /// The parallel search operation will take ownership of this monitor: the monitor is disposed when the search finishes! + /// Observable that starts performing the search when subscribed to. Does not support more than one subscriber! public static IObservable FindAllParallel(ISearchStrategy strategy, SearchLocation location, IProgressMonitor progressMonitor) { currentSearchRegion = null; @@ -122,7 +130,7 @@ namespace SearchAndReplace observer.OnError(t.Exception); else observer.OnCompleted(); - this.Dispose(); + monitor.Dispose(); }); return this; } @@ -181,8 +189,9 @@ namespace SearchAndReplace public void Dispose() { + // Warning: Dispose() may be called concurrently while the operation is still in progress. + // We cannot dispose the progress monitor here because it is still in use by the search operation. cts.Cancel(); - monitor.Dispose(); } SearchedFile SearchFile(FileName fileName) @@ -509,6 +518,12 @@ namespace SearchAndReplace && result.Length == editor.SelectionLength; } + /// + /// Shows searchs results in the search results pad, and brings that pad to front. + /// + /// The pattern that is being searched for; used for the title of the search in the list of past searched. + /// An observable that represents a background search operation. + /// The search results pad will subscribe to this observable in order to receive the search results. public static void ShowSearchResults(string pattern, IObservable results) { string title = StringParser.Parse("${res:MainWindow.Windows.SearchResultPanel.OccurrencesOf}", diff --git a/src/Main/Base/Project/Editor/Search/ISearchResultFactory.cs b/src/Main/Base/Project/Editor/Search/ISearchResultFactory.cs index cf55789a23..73469bdbf5 100644 --- a/src/Main/Base/Project/Editor/Search/ISearchResultFactory.cs +++ b/src/Main/Base/Project/Editor/Search/ISearchResultFactory.cs @@ -12,8 +12,18 @@ namespace ICSharpCode.SharpDevelop.Editor.Search /// public interface ISearchResultFactory { + /// + /// Creates an object from a list of matches. + /// + /// The title of the search. + /// The list of matches. CreateSearchResult() will enumerate once through the IEnumerable in order to retrieve the search results. ISearchResult CreateSearchResult(string title, IEnumerable matches); + /// + /// Creates an object for a background search operation. + /// + /// The title of the search. + /// The background search operation. CreateSearchResult() will subscribe to the observable in order to retrieve the search results. ISearchResult CreateSearchResult(string title, IObservable matches); } } diff --git a/src/Main/Base/Project/Editor/Search/SearchResultsPad.cs b/src/Main/Base/Project/Editor/Search/SearchResultsPad.cs index 59c2002fee..e255199f30 100644 --- a/src/Main/Base/Project/Editor/Search/SearchResultsPad.cs +++ b/src/Main/Base/Project/Editor/Search/SearchResultsPad.cs @@ -78,6 +78,10 @@ namespace ICSharpCode.SharpDevelop.Editor.Search SD.WinForms.SetContent(contentPlaceholder, null); } + /// + /// Shows a search in the search results pad. + /// The previously shown search will be stored in the list of past searches. + /// public void ShowSearchResults(ISearchResult result) { if (result == null) @@ -108,11 +112,23 @@ namespace ICSharpCode.SharpDevelop.Editor.Search SearchResultsShown(this, EventArgs.Empty); } + /// + /// Shows a search in the search results pad. + /// The previously shown search will be stored in the list of past searches. + /// + /// The title of the search. + /// The list of matches. ShowSearchResults() will enumerate once through the IEnumerable in order to retrieve the search results. public void ShowSearchResults(string title, IEnumerable matches) { ShowSearchResults(CreateSearchResult(title, matches)); } + /// + /// Performs a background search in the search results pad. + /// The previously shown search will be stored in the list of past searches. + /// + /// The title of the search. + /// The background search operation. ShowSearchResults() will subscribe to the observable in order to retrieve the search results. public void ShowSearchResults(string title, IObservable matches) { ShowSearchResults(CreateSearchResult(title, matches)); @@ -120,6 +136,7 @@ namespace ICSharpCode.SharpDevelop.Editor.Search public event EventHandler SearchResultsShown = delegate {}; + /// public static ISearchResult CreateSearchResult(string title, IEnumerable matches) { if (title == null) @@ -135,6 +152,7 @@ namespace ICSharpCode.SharpDevelop.Editor.Search } + /// public static ISearchResult CreateSearchResult(string title, IObservable matches) { if (title == null) diff --git a/src/Main/Base/Project/Util/IProgressMonitor.cs b/src/Main/Base/Project/Util/IProgressMonitor.cs index c9428b5a4d..0dfa7b8519 100644 --- a/src/Main/Base/Project/Util/IProgressMonitor.cs +++ b/src/Main/Base/Project/Util/IProgressMonitor.cs @@ -39,7 +39,7 @@ namespace ICSharpCode.SharpDevelop /// /// The amount of work this sub-task performs in relation to the work of this task. /// That means, this parameter is used as a scaling factor for work performed within the subtask. - /// + /// /// A cancellation token that can be used to cancel the sub-task. /// Note: cancelling the main task will not cancel the sub-task. /// diff --git a/src/Main/Base/Project/Util/ProgressCollector.cs b/src/Main/Base/Project/Util/ProgressCollector.cs index 09cad86577..4aa84ae7f0 100644 --- a/src/Main/Base/Project/Util/ProgressCollector.cs +++ b/src/Main/Base/Project/Util/ProgressCollector.cs @@ -191,6 +191,9 @@ namespace ICSharpCode.SharpDevelop { lock (namedMonitors) { bool wasFirst = namedMonitors.First == nameEntry; + // Note: if Remove() crashes with "InvalidOperationException: The LinkedList node does not belong to current LinkedList.", + // that's an indication that the progress monitor is being disposed multiple times concurrently. + // (which is not allowed according to IProgressMonitor thread-safety documentation) namedMonitors.Remove(nameEntry); if (wasFirst) SetTaskName(namedMonitors.First != null ? namedMonitors.First.Value : null);