Merge pull request #1776 from github/fixes/pr-list-errors-2

Make PR list handle errors more gracefully.

Author: grokys

src/GitHub.App/Collections/SequentialListSource.cs

@@ -146,11 +146,18 @@ async Task<Page<TModel>> EnsureLoaded(int pageNumber)
                     }
                 }
 
-                await Task.WhenAny(loading, pageLoaded).ConfigureAwait(false);
+                var completed = await Task.WhenAny(loading, pageLoaded).ConfigureAwait(false);
+
+                if (completed.IsFaulted)
+                {
+                    throw completed.Exception;
+                }
 
                 if (pageLoaded.IsCompleted)
                 {
-                    return pages[pageNumber];
+                    // A previous waiting task may have already returned the page. If so, return null.
+                    pages.TryGetValue(pageNumber, out var result);
+                    return result;
                 }
             }
 
@@ -177,13 +184,18 @@ async Task Load()
         {
             OnBeginLoading();
 
-            while (nextPage <= loadTo && !disposed)
+            try
+            {
+                while (nextPage <= loadTo && !disposed)
+                {
+                    await LoadNextPage().ConfigureAwait(false);
+                    PageLoaded?.Invoke(this, EventArgs.Empty);
+                }
+            }
+            finally
             {
-                await LoadNextPage().ConfigureAwait(false);
-                PageLoaded?.Invoke(this, EventArgs.Empty);
+                OnEndLoading();
             }
-
-            OnEndLoading();
         }
 
         async Task LoadNextPage()

src/GitHub.App/Collections/VirtualizingList.cs

@@ -3,7 +3,9 @@
 using System.Collections.Generic;
 using System.Collections.Specialized;
 using System.ComponentModel;
+using System.IO;
 using System.Linq;
+using System.Reflection;
 using System.Threading.Tasks;
 using System.Windows;
 using System.Windows.Threading;
@@ -139,6 +141,7 @@ object IList.this[int index]
 
         public event NotifyCollectionChangedEventHandler CollectionChanged;
         public event PropertyChangedEventHandler PropertyChanged;
+        public event EventHandler<ErrorEventArgs> InitializationError;
 
         public IEnumerator<T> GetEnumerator()
         {
@@ -176,14 +179,22 @@ void LoadCount()
                 {
                     countTask.ContinueWith(x =>
                     {
-                        count = x.Result;
-                        SendReset();
-                        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Count)));
+                        if (x.IsFaulted)
+                        {
+                            RaiseInitializationError(x.Exception);
+                        }
+                        else if (!x.IsCanceled)
+                        {
+                            count = x.Result;
+                            SendReset();
+                            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Count)));
+                        }
                     }, TaskScheduler.FromCurrentSynchronizationContext());
                 }
             }
             catch (Exception ex)
             {
+                RaiseInitializationError(ex);
                 log.Error(ex, "Error loading virtualizing list count");
             }
         }
@@ -206,6 +217,25 @@ async void LoadPage(int number)
             catch (Exception ex)
             {
                 log.Error(ex, "Error loading virtualizing list page {Number}", number);
+                pages.Remove(number);
+            }
+        }
+
+        void RaiseInitializationError(Exception e)
+        {
+            if (InitializationError != null)
+            {
+                if (e is AggregateException ae)
+                {
+                    e = ae = ae.Flatten();
+
+                    if (ae.InnerExceptions.Count == 1)
+                    {
+                        e = ae.InnerException;
+                    }
+                }
+
+                InitializationError(this, new ErrorEventArgs(e));
             }
         }
 

src/GitHub.App/ViewModels/GitHubPane/IssueListViewModelBase.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
+using System.IO;
 using System.Linq;
 using System.Reactive;
 using System.Reactive.Disposables;
@@ -197,6 +198,7 @@ public override Task Refresh()
             view.Filter = FilterItem;
             Items = items;
             ItemsView = view;
+            Error = null;
 
             dispose.Add(itemSource);
             dispose.Add(
@@ -208,6 +210,11 @@ public override Task Refresh()
                     this.WhenAnyValue(x => x.AuthorFilter.Selected),
                     (loading, count, _, __, ___) => Tuple.Create(loading, count))
                 .Subscribe(x => UpdateState(x.Item1, x.Item2)));
+            dispose.Add(
+                Observable.FromEventPattern<ErrorEventArgs>(
+                    x => items.InitializationError += x,
+                    x => items.InitializationError -= x)
+                .Subscribe(x => Error = x.EventArgs.GetException()));
             subscription = dispose;
 
             return Task.CompletedTask;

src/GitHub.App/ViewModels/GitHubPane/PullRequestListViewModel.cs

@@ -122,7 +122,7 @@ protected override IIssueListItemViewModelBase CreateViewModel(PullRequestListIt
                 return result;
             }
 
-            protected override Task<Page<PullRequestListItemModel>> LoadPage(string after)
+            protected override async Task<Page<PullRequestListItemModel>> LoadPage(string after)
             {
                 PullRequestStateEnum[] states;
 
@@ -139,15 +139,12 @@ protected override Task<Page<PullRequestListItemModel>> LoadPage(string after)
                         break;
                 }
 
-                var sw = Stopwatch.StartNew();
-                var result = owner.service.ReadPullRequests(
+                var result = await owner.service.ReadPullRequests(
                     HostAddress.Create(owner.RemoteRepository.CloneUrl),
                     owner.RemoteRepository.Owner,
                     owner.RemoteRepository.Name,
                     after,
-                    states);
-                sw.Stop();
-                System.Diagnostics.Debug.WriteLine("Read PR page in " + sw.Elapsed);
+                    states).ConfigureAwait(false);
                 return result;
             }
         }

src/GitHub.App/ViewModels/UserFilterViewModel.cs

@@ -120,7 +120,13 @@ async Task Load()
             while (true)
             {
                 var page = await load(after);
-                users.AddRange(page.Items.Select(x => new ActorViewModel(x)));
+
+                foreach (var item in page.Items)
+                {
+                    var vm = new ActorViewModel(item);
+                    users.Add(vm);
+                }
+
                 after = page.EndCursor;
                 if (!page.HasNextPage) break;
             }

test/GitHub.App.UnitTests/Collections/SequentialListSourceTests.cs

@@ -0,0 +1,132 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reactive;
+using System.Reactive.Linq;
+using System.Reactive.Subjects;
+using System.Reactive.Threading.Tasks;
+using System.Threading.Tasks;
+using GitHub.Collections;
+using GitHub.Models;
+using NUnit.Framework;
+
+namespace GitHub.App.UnitTests.Collections
+{
+    public class SequentialListSourceTests
+    {
+        [Test]
+        public async Task GetCount_Should_Load_First_Page()
+        {
+            var target = new TestSource();
+
+            Assert.That(target.PagesLoaded, Is.Empty);
+
+            var count = await target.GetCount();
+
+            Assert.That(count, Is.EqualTo(100));
+            Assert.That(target.PagesLoaded, Is.EqualTo(new[] { 0 }));
+        }
+
+        [Test]
+        public async Task GetPage_Should_Load_Pages()
+        {
+            var target = new TestSource();
+
+            Assert.That(target.PagesLoaded, Is.Empty);
+
+            var count = await target.GetPage(3);
+
+            Assert.That(target.PagesLoaded, Is.EqualTo(new[] { 0, 1, 2, 3 }));
+        }
+
+        [Test]
+        public void GetPage_Should_Stop_Loading_Pages_When_LoadPage_Throws()
+        {
+            var target = new TestSource(2);
+
+            Assert.That(target.PagesLoaded, Is.Empty);
+
+            Assert.ThrowsAsync<AggregateException>(() => target.GetPage(3));
+            Assert.That(target.PagesLoaded, Is.EqualTo(new[] { 0, 1 }));
+        }
+
+        [Test]
+        public void IsLoading_Should_Be_Set_To_False_When_LoadPage_Throws()
+        {
+            var target = new TestSource(2);
+
+            Assert.That(target.PagesLoaded, Is.Empty);
+
+            Assert.ThrowsAsync<AggregateException>(() => target.GetPage(3));
+            Assert.That(target.IsLoading, Is.False);
+        }
+
+        [Test]
+        public async Task Should_Not_Load_Duplicate_Pages()
+        {
+            var trigger = new Subject<Unit>();
+            var target = new TestSource(loadTrigger: trigger);
+
+            var task1 = target.GetPage(1);
+            var task2 = target.GetPage(1);
+
+            Assert.That(target.PagesLoaded, Is.Empty);
+
+            trigger.OnNext(Unit.Default);
+            trigger.OnNext(Unit.Default);
+
+            await task1;
+            await task2;
+
+            Assert.That(target.PagesLoaded, Is.EqualTo(new[] { 0, 1 }));
+        }
+
+        class TestSource : SequentialListSource<string, string>
+        {
+            const int PageCount = 10;
+            readonly int? throwAtPage;
+            readonly ISubject<Unit> loadTrigger;
+
+            public TestSource(
+                int? throwAtPage = null,
+                ISubject<Unit> loadTrigger = null)
+            {
+                this.throwAtPage = throwAtPage;
+                this.loadTrigger = loadTrigger;
+            }
+
+            public override int PageSize => 10;
+            public List<int> PagesLoaded { get; private set; } = new List<int>();
+
+            protected override string CreateViewModel(string model)
+            {
+                return model + " loaded";
+            }
+
+            protected override async Task<Page<string>> LoadPage(string after)
+            {
+                var page = after != null ? int.Parse(after) : 0;
+
+                if (loadTrigger != null)
+                {
+                    await loadTrigger.Take(1).ToTask().ConfigureAwait(false);
+                }
+
+                if (page == throwAtPage)
+                {
+                    throw new GitHubLogicException("Thrown.");
+                }
+
+                PagesLoaded.Add(page);
+
+                return new Page<string>
+                {
+                    EndCursor = (page + 1).ToString(),
+                    HasNextPage = page < PageCount,
+                    Items = Enumerable.Range(page * PageSize, PageSize).Select(x => "Item " + x).ToList(),
+                    TotalCount = PageSize * PageCount,
+                };
+            }
+        }
+    }
+}

test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj

@@ -157,6 +157,7 @@
     <Compile Include="..\Helpers\TestSharedCache.cs" />
     <Compile Include="Args.cs" />
     <Compile Include="Caches\ImageCacheTests.cs" />
+    <Compile Include="Collections\SequentialListSourceTests.cs" />
     <Compile Include="Factories\ModelServiceFactoryTests.cs" />
     <Compile Include="Models\AccountModelTests.cs" />
     <Compile Include="Models\ModelServiceTests.cs" />