7
\$\begingroup\$

Our Rubberduck VBA IDE Add In does some static code analysis, and then reports all of the found issues back in a gridview. This analysis can take a long time for larger projects and the UI appeared to be frozen until all of the analysis was complete. I've corrected that issue by updating the number of issues found as they're found and now I'm wondering if there is anything else in this code I can improve prior to merging the branch in.

I'm just learning about async code, so I'm particularly interested in how we've done that. Some of it feels wrong and dirty, but I don't know well enough to be entirely sure. I did my best to apply what I learned in a recent review of some other code.

For reference, this is what the UI looks like.

Code Inspection Results

I extracted the Inspection logic into it's own class and interface to inject into the presenter along with the view. The idea is to have the presenter listen for issues being found by the inspector and update the GUI asynchronously, while the inspector in turn asynchronously searchers for issues in the VBProject.

Headers link to this particular version of the files on GitHub and I can add any code not here that you find relevant upon request.

IInspector:

using System;
using System.Collections.Generic;
using Microsoft.Vbe.Interop;
using System.Threading.Tasks;

namespace Rubberduck.Inspections
{
    public interface IInspector
    {
        Task<IList<ICodeInspectionResult>> FindIssues(VBProject project);
        event EventHandler<InspectorIssuesFoundEventArg> IssuesFound;
    }

    public class InspectorIssuesFoundEventArg : EventArgs
    {
        private readonly int _count;
        public int Count { get { return _count; } }

        public InspectorIssuesFoundEventArg(int count)
        {
            _count = count;
        }
    }
}

Inspector:

using Microsoft.Vbe.Interop;
using Rubberduck.VBA;
using Rubberduck.VBA.Nodes;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

namespace Rubberduck.Inspections
{
    public class Inspector : IInspector
    {
        private readonly IRubberduckParser _parser;
        private readonly IList<IInspection> _inspections;

        public Inspector(IRubberduckParser parser, IEnumerable<IInspection> inspections)
        {
            _parser = parser;
            _inspections = inspections.ToList();
        }

        public async Task<IList<ICodeInspectionResult>> FindIssues(VBProject project)
        {
            await Task.Yield();

            var code = new VBProjectParseResult(_parser.Parse(project));
            var results = new ConcurrentBag<ICodeInspectionResult>();

            var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
                .Select(inspection =>
                    new Task(() =>
                    {
                        var result = inspection.GetInspectionResults(code);
                        var count = result.Count();
                        if (count > 0)
                        {
                            RaiseIssuesFound(count);

                            foreach (var inspectionResult in result)
                            {
                                results.Add(inspectionResult);
                            }
                        }
                    })).ToArray();

            foreach (var inspection in inspections)
            {
                inspection.Start();
            }

            Task.WaitAll(inspections);

            return results.ToList();
        }

        public event EventHandler<InspectorIssuesFoundEventArg> IssuesFound;
        private void RaiseIssuesFound(int count)
        {
            var handler = IssuesFound;
            if (handler == null)
            {
                return;
            }

            var args = new InspectorIssuesFoundEventArg(count);
            handler(this, args);
        }
    }
}

CodeInspectionDockablePresenter:

using Microsoft.Vbe.Interop;
using Rubberduck.Extensions;
using Rubberduck.Inspections;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Windows.Forms;

namespace Rubberduck.UI.CodeInspections
{
    public class CodeInspectionsDockablePresenter : DockablePresenterBase
    {
        private CodeInspectionsWindow Control { get { return UserControl as CodeInspectionsWindow; } }

        private IList<ICodeInspectionResult> _results;
        private IInspector _inspector;

        public CodeInspectionsDockablePresenter(IInspector inspector, VBE vbe, AddIn addin, CodeInspectionsWindow window)
            :base(vbe, addin, window)
        {
            _inspector = inspector;

            _inspector.IssuesFound += OnIssuesFound;

            Control.RefreshCodeInspections += OnRefreshCodeInspections;
            Control.NavigateCodeIssue += OnNavigateCodeIssue;
            Control.QuickFix += OnQuickFix;
        }

        private void OnIssuesFound(object sender, InspectorIssuesFoundEventArg e)
        {
            var newCount = Control.IssueCount + e.Count;
            Control.IssueCount = newCount;
            Control.IssueCountText = string.Format("{0} issue" + (newCount > 1 ? "s" : string.Empty), newCount);
        }

        private void OnQuickFix(object sender, QuickFixEventArgs e)
        {
            e.QuickFix(VBE);
            OnRefreshCodeInspections(null, EventArgs.Empty);
        }

        public override void Show()
        {
            base.Show();

            if (VBE.ActiveVBProject != null)
            {
                OnRefreshCodeInspections(this, EventArgs.Empty);
            }
        }

        private void OnNavigateCodeIssue(object sender, NavigateCodeEventArgs e)
        {
            try
            {
                var location = VBE.FindInstruction(e.QualifiedName, e.Selection);
                location.CodeModule.CodePane.SetSelection(e.Selection);

                var codePane = location.CodeModule.CodePane;
                var selection = location.Selection;
                codePane.SetSelection(selection);
            }
            catch (Exception exception)
            {
                System.Diagnostics.Debug.Assert(false, exception.ToString());
            }
        }

        private void OnRefreshCodeInspections(object sender, EventArgs e)
        {
            Refresh();
        }

        private async void Refresh()
        {
            Control.Cursor = Cursors.WaitCursor;

            try
            {
                Control.IssueCount = 0;
                Control.IssueCountText = "0 issues";
                Control.InspectionResults.Clear();

                _results = await this._inspector.FindIssues(VBE.ActiveVBProject);
                Control.SetContent(_results.Select(item => new CodeInspectionResultGridViewItem(item)).OrderBy(item => item.Component).ThenBy(item => item.Line));
            }
            finally
            {
                Control.Cursor = Cursors.Default;
            }
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$
  1. You are still using the UI thread to do the project parsing _parser.Parse(project), because Task.Yield will return the execution back to UI thread. If this call may take significant time, I would suggest to replace it with Task.Run.

  2. It looks like you should get exceptions in OnIssuesFound, as you're updating the UI from non-UI thread. See the fix below.

  3. What you do with IssuesFound event is actually reporting progress. .NET has a built-in support for asynchronous progress reporting via IProgress<T> and Progress<T>, which properly handles the synchronization with UI thread. See description of how to use them here: Enabling Progress and Cancellation in Async APIs

  4. Instead of new Task() and then task.Start you can just use the Task.Run method to create and run the task.

  5. Don't use Task.WaitAll as it blocks the thread until all tasks complete. Use await Task.WhenAll() instead.

As a result, my take on FindIssues refactoring look like this:

public async Task<IList<ICodeInspectionResult>> FindIssuesAsync(VBProject project, IProgress<int> progress)
{
    var code = await Task.Run(() => new VBProjectParseResult(_parser.Parse(project))).ConfigureAwait(false);

    var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
                .Select(inspection =>
                    Task.Run(() =>
                    {
                        var result = inspection.GetInspectionResults(code).ToArray();
                        var count = result.Count();
                        if (progress != null && count > 0)
                            progress.Report(count);

                        return result;
                    }));

    ICodeInspectionResult[][] results = await Task.WhenAll(inspections);

    return results.SelectMany(enumerable => enumerable).ToList();
}
\$\endgroup\$
  • \$\begingroup\$ I'm not getting exceptions of any kind though. The code seems to be working just fine. \$\endgroup\$ – RubberDuck Mar 2 '15 at 0:14
  • \$\begingroup\$ Task.Run spins up a new thread, I wouldn't do that unless I know that the work is CPU bound. Task.Yield will return the call to the calling thread and place the work onto another thread in the threadpool or spin it up, it will be async wrt to the UI. I agree with await Task.WhenAll \$\endgroup\$ – JasonLind Mar 2 '15 at 3:42
  • 1
    \$\begingroup\$ @JasonLind Awaiting Task.Yield will put the rest of the work on the SynchronizationContext corresponding to the UI thread, i.e. rest of the method will run on UI thread. I don't think it is the intention. Task.Run does not necessarily spin up a thread, it schedules the execution on the ThreadPool \$\endgroup\$ – almaz Mar 2 '15 at 10:16
  • \$\begingroup\$ @RubberDuck I'm just looking at your code, didn't run it. You're raising IssuesFound event from the thread pool (not UI thread), and directly update the UI from the event handler. Anyway, IProgress<T> is the better way to report progress to the UI thread. \$\endgroup\$ – almaz Mar 2 '15 at 10:38
  • \$\begingroup\$ Yeah... Idk. I do agree that IProgress<T> is called for here though. Thanks for pointing me to that. \$\endgroup\$ – RubberDuck Mar 2 '15 at 10:40
2
\$\begingroup\$

Talking about this snippet

var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
    .Select(inspection =>
        new Task(() =>
        {
            var result = inspection.GetInspectionResults(code);
            var count = result.Count();
            if (count > 0)
            {
                RaiseIssuesFound(count);

                foreach (var inspectionResult in result)
                {
                    results.Add(inspectionResult);
                }
            }
        })).ToArray();  

Calling inspection.GetInspectionResults() returns an IEnumerable<T> so if you are calling Count() on the result, this call is iterating over the IEnumerable and later you are iterating again if count > 0.

A better approach is to use Any() and rearange the code to call RaiseIssuesFound() after iterating over the IEnumerable.

Because an IEnumerable indicates a kind of "collection" one should name the variable holding this by using the pluralform.

var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
    .Select(inspection =>
        new Task(() =>
        {
            var inspectionResults = inspection.GetInspectionResults(code);
            if (inspectionResults.Any())
            {
                var count = 0;
                foreach (var inspectionResult in inspectionResults)
                {
                    results.Add(inspectionResult);
                    count++;
                }
                RaiseIssuesFound(count);
            }
        })).ToArray();
\$\endgroup\$
2
\$\begingroup\$

I would not use Task.Start as that kicks off a new thread (unless you know the work is CPU bound), I'd use async/await in the following manor:

 var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
            .Select(inspection =>ProcessInspection(inspection, progress)).ToArray();
private async Task<ICodeInspectionResult[]> ProcessInspection(IInspection inspection, IProgress<int> progress)
{
   await Task.Yield();
   var result = inspection.GetInspectionResults(code).ToArray();
   var count = result.Count();
   if (progress != null && count > 0)
       progress.Report(count);

   return result;

}

A thread may or may not be spun up based on the runtime threadpool requirements, you shouldn't spin up threads directly unless you know the work is CPU bound.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.