Merge pull request #362 from github/fixes/stackoverflow

shana · web-flow · commit f8dd285bfa36 · 2016-06-16T13:27:16.000+02:00
Fix stackoverflow and resource disposal
diff --git a/src/GitHub.App/Caches/ImageCache.cs b/src/GitHub.App/Caches/ImageCache.cs
@@ -213,33 +213,8 @@ static string GetCacheKey(Uri url)
             return url.ToString();
         }
 
-        bool disposed;
-        void Dispose(bool disposing)
-        {
-            if (disposing)
-            {
-                if (disposed) return;
-
-                try
-                {
-                    cacheFactory.Select(x =>
-                    {
-                        x.Dispose();
-                        return x.Shutdown;
-                    }).Wait();
-                }
-                catch (Exception e)
-                {
-                    log.Warn("Exception occured while disposing ImageCache", e);
-                }
-                disposed = true;
-            }
-        }
-
         public void Dispose()
-        {
-            Dispose(true);
-        }
+        {}
 
         class UriComparer : IEqualityComparer<Uri>
         {
diff --git a/src/GitHub.App/Caches/LoginCache.cs b/src/GitHub.App/Caches/LoginCache.cs
@@ -56,28 +56,7 @@ public IObservable<Unit> Flush()
             return cache.Secure.Flush();
         }
 
-        bool disposed;
-        void Dispose(bool disposing)
-        {
-            if (disposing)
-            {
-                if (disposed) return;
-
-                try
-                {
-                    cache.Dispose();
-                }
-                catch (Exception e)
-                {
-                    log.Warn("Exception occurred while disposing shared cache", e);
-                }
-                disposed = true;
-            }
-        }
-
         public void Dispose()
-        {
-            Dispose(true);
-        }
+        {}
     }
 }
diff --git a/src/GitHub.App/Factories/HostCacheFactory.cs b/src/GitHub.App/Factories/HostCacheFactory.cs
@@ -40,17 +40,8 @@ public IBlobCache Create(HostAddress hostAddress)
         IOperatingSystem OperatingSystem { get { return operatingSystem.Value; } }
         IBlobCacheFactory BlobCacheFactory { get { return blobCacheFactory.Value; } }
 
-        bool disposed;
         protected virtual void Dispose(bool disposing)
-        {
-            if (disposing)
-            {
-                if (disposed) return;
-                disposed = true;
-                if (blobCacheFactory.IsValueCreated)
-                    blobCacheFactory.Value.Dispose();
-            }
-        }
+        {}
 
         public void Dispose()
         {
diff --git a/src/GitHub.App/Factories/RepositoryHostFactory.cs b/src/GitHub.App/Factories/RepositoryHostFactory.cs
@@ -5,6 +5,7 @@
 using GitHub.Models;
 using GitHub.Primitives;
 using GitHub.Services;
+using System.Reactive.Disposables;
 
 namespace GitHub.Factories
 {
@@ -17,6 +18,7 @@ public class RepositoryHostFactory : IRepositoryHostFactory
         readonly ILoginCache loginCache;
         readonly IAvatarProvider avatarProvider;
         readonly ITwoFactorChallengeHandler twoFactorChallengeHandler;
+        readonly CompositeDisposable hosts = new CompositeDisposable();
 
         [ImportingConstructor]
         public RepositoryHostFactory(
@@ -38,8 +40,14 @@ public IRepositoryHost Create(HostAddress hostAddress)
             var apiClient = apiClientFactory.Create(hostAddress);
             var hostCache = hostCacheFactory.Create(hostAddress);
             var modelService = new ModelService(apiClient, hostCache, avatarProvider);
+            var host = new RepositoryHost(apiClient, modelService, loginCache, twoFactorChallengeHandler);
+            hosts.Add(host);
+            return host;
+        }
 
-            return new RepositoryHost(apiClient, modelService, loginCache, twoFactorChallengeHandler);
+        public void Remove(IRepositoryHost host)
+        {
+            hosts.Remove(host);
         }
 
         bool disposed;
@@ -48,11 +56,8 @@ protected virtual void Dispose(bool disposing)
             if (disposing)
             {
                 if (disposed) return;
-
-                loginCache.Dispose();
-                avatarProvider.Dispose();
-                hostCacheFactory.Dispose();
                 disposed = true;
+                hosts.Dispose();
             }
         }
 
diff --git a/src/GitHub.App/Factories/SqlitePersistentBlobCacheFactory.cs b/src/GitHub.App/Factories/SqlitePersistentBlobCacheFactory.cs
@@ -4,15 +4,15 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel.Composition;
-using System.Reactive.Disposables;
+using System.Reactive.Linq;
+using System.Threading.Tasks;
 
 namespace GitHub.Factories
 {
     [Export(typeof(IBlobCacheFactory))]
     [PartCreationPolicy(CreationPolicy.Shared)]
     public class SqlitePersistentBlobCacheFactory : IBlobCacheFactory
     {
-        readonly CompositeDisposable disposables = new CompositeDisposable();
         static readonly Logger log = LogManager.GetCurrentClassLogger();
         Dictionary<string, IBlobCache> cache = new Dictionary<string, IBlobCache>();
 
@@ -26,7 +26,6 @@ public IBlobCache CreateBlobCache(string path)
             {
                 var c = new SQLitePersistentBlobCache(path);
                 cache.Add(path, c);
-                disposables.Add(c);
                 return c;
             }
             catch(Exception ex)
@@ -43,7 +42,14 @@ protected virtual void Dispose(bool disposing)
             {
                 if (disposed) return;
                 disposed = true;
-                disposables.Dispose();
+                Task.Run(() =>
+                {
+                    foreach (var c in cache.Values)
+                    {
+                        c.Dispose();
+                        c.Shutdown.Wait();
+                    }
+                }).Wait(500);
             }
         }
 
diff --git a/src/GitHub.App/Factories/UIFactory.cs b/src/GitHub.App/Factories/UIFactory.cs
@@ -35,6 +35,16 @@ public IUIPair CreateViewAndViewModel(UIViewType viewType)
         {
             return new UIPair(viewType, factory.GetView(viewType), factory.GetViewModel(viewType));
         }
+
+        protected virtual void Dispose(bool disposing)
+        {}
+
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
     }
 
     /// <summary>
diff --git a/src/GitHub.App/Models/RepositoryHost.cs b/src/GitHub.App/Models/RepositoryHost.cs
@@ -268,24 +268,8 @@ IObservable<UserAndScopes> GetUserFromApi()
             return Observable.Defer(() => ApiClient.GetUser());
         }
 
-        bool disposed;
         protected virtual void Dispose(bool disposing)
-        {
-            if (disposing)
-            {
-                if (disposed) return;
-
-                try
-                {
-                    ModelService.Dispose();
-                }
-                catch (Exception e)
-                {
-                    log.Warn("Exception occured while disposing RepositoryHost's ModelService", e);
-                }
-                disposed = true;
-            }
-        }
+        {}
 
         public void Dispose()
         {
diff --git a/src/GitHub.App/Models/RepositoryHosts.cs b/src/GitHub.App/Models/RepositoryHosts.cs
@@ -39,7 +39,6 @@ public RepositoryHosts(
             this.connectionManager = connectionManager;
 
             RepositoryHostFactory = repositoryHostFactory;
-            disposables.Add(repositoryHostFactory);
             GitHubHost = DisconnectedRepositoryHost;
             EnterpriseHost = DisconnectedRepositoryHost;
 
@@ -101,9 +100,7 @@ public RepositoryHosts(
                 {
                     var host = LookupHost(x.HostAddress);
                     if (host.Address != x.HostAddress)
-                    {
                         host = RepositoryHostFactory.Create(x.HostAddress);
-                    }
                     return host;
                 })
                 .Select(h => LogOut(h))
@@ -147,7 +144,6 @@ public IObservable<AuthenticationResult> LogIn(
         {
             var isDotCom = HostAddress.GitHubDotComHostAddress == address;
             var host = RepositoryHostFactory.Create(address);
-            disposables.Add(host);
             return host.LogIn(usernameOrEmail, password)
                 .Catch<AuthenticationResult, Exception>(Observable.Throw<AuthenticationResult>)
                 .Do(result =>
@@ -180,7 +176,6 @@ public IObservable<AuthenticationResult> LogInFromCache(HostAddress address)
         {
             var isDotCom = HostAddress.GitHubDotComHostAddress == address;
             var host = RepositoryHostFactory.Create(address);
-            disposables.Add(host);
             return host.LogInFromCache()
                 .Catch<AuthenticationResult, Exception>(Observable.Throw<AuthenticationResult>)
                 .Do(result =>
@@ -210,7 +205,7 @@ public IObservable<Unit> LogOut(IRepositoryHost host)
                     else
                         EnterpriseHost = null;
                     connectionManager.RemoveConnection(address);
-                    disposables.Remove(host);
+                    RepositoryHostFactory.Remove(host);
                 });
         }
 
diff --git a/src/GitHub.App/Services/AvatarProvider.cs b/src/GitHub.App/Services/AvatarProvider.cs
@@ -77,16 +77,8 @@ BitmapImage DefaultAvatar(IAvatarContainer apiAccount)
             return apiAccount.IsUser ? DefaultUserBitmapImage : DefaultOrgBitmapImage;
         }
 
-        bool disposed;
         protected virtual void Dispose(bool disposing)
-        {
-            if (disposing)
-            {
-                if (disposed) return;
-                imageCache.Dispose();
-                disposed = true;
-            }
-        }
+        {}
 
         public void Dispose()
         {
diff --git a/src/GitHub.App/Services/ModelService.cs b/src/GitHub.App/Services/ModelService.cs
@@ -280,15 +280,8 @@ public IObservable<Unit> InsertUser(AccountCacheItem user)
             return hostCache.InsertObject("user", user);
         }
 
-        bool disposed;
         protected virtual void Dispose(bool disposing)
-        {
-            if (disposing)
-            {
-                if (disposed) return;
-                disposed = true;
-            }
-        }
+        {}
 
         public void Dispose()
         {
diff --git a/src/GitHub.App/ViewModels/RepositoryCreationViewModel.cs b/src/GitHub.App/ViewModels/RepositoryCreationViewModel.cs
@@ -44,16 +44,14 @@ public class RepositoryCreationViewModel : RepositoryFormViewModel, IRepositoryC
         RepositoryCreationViewModel(
             IConnectionRepositoryHostMap connectionRepositoryHostMap,
             IOperatingSystem operatingSystem,
-            IRepositoryCreationService repositoryCreationService,
-            IAvatarProvider avatarProvider)
-            : this(connectionRepositoryHostMap.CurrentRepositoryHost, operatingSystem, repositoryCreationService, avatarProvider)
+            IRepositoryCreationService repositoryCreationService)
+            : this(connectionRepositoryHostMap.CurrentRepositoryHost, operatingSystem, repositoryCreationService)
         {}
 
         public RepositoryCreationViewModel(
             IRepositoryHost repositoryHost,
             IOperatingSystem operatingSystem,
-            IRepositoryCreationService repositoryCreationService,
-            IAvatarProvider avatarProvider)
+            IRepositoryCreationService repositoryCreationService)
         {
             this.repositoryHost = repositoryHost;
             this.operatingSystem = operatingSystem;
diff --git a/src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs b/src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs
@@ -117,13 +117,11 @@ public IObservable<T> Listen(IObservable<T> obs)
             sourceQueue = obs
                 .Do(data => queue.Enqueue(new ActionData(data)));
 
-            source = Observable
-                .Generate(StartQueue(),
-                    i => !disposed,
-                    i => i + 1,
-                    i => GetFromQueue(),
-                    i => delay
-                )
+            source = Observable.Defer(() => {
+                StartQueue();
+                return Observable.Timer(TimeSpan.Zero, delay)
+                                 .Select(_ => GetFromQueue());
+            })
                 .Where(data => data.Item != null)
                 .ObserveOn(scheduler)
                 .Select(x => ProcessItem(x, original))
diff --git a/src/GitHub.Exports.Reactive/Factories/IApiClientFactory.cs b/src/GitHub.Exports.Reactive/Factories/IApiClientFactory.cs
@@ -1,5 +1,6 @@
 ﻿using GitHub.Api;
 using GitHub.Primitives;
+using System;
 
 namespace GitHub.Factories
 {
diff --git a/src/GitHub.Exports.Reactive/Factories/IRepositoryHostFactory.cs b/src/GitHub.Exports.Reactive/Factories/IRepositoryHostFactory.cs
@@ -7,5 +7,6 @@ namespace GitHub.Factories
     public interface IRepositoryHostFactory : IDisposable
     {
         IRepositoryHost Create(HostAddress hostAddress);
+        void Remove(IRepositoryHost host);
     }
 }
diff --git a/src/GitHub.Exports/Factories/IUIFactory.cs b/src/GitHub.Exports/Factories/IUIFactory.cs
@@ -1,8 +1,9 @@
 ﻿using GitHub.Exports;
+using System;
 
 namespace GitHub.App.Factories
 {
-    public interface IUIFactory
+    public interface IUIFactory : IDisposable
     {
         IUIPair CreateViewAndViewModel(UIViewType viewType);
     }
diff --git a/src/UnitTests/GitHub.App/ViewModels/RepositoryCreationViewModelTests.cs b/src/UnitTests/GitHub.App/ViewModels/RepositoryCreationViewModelTests.cs
@@ -33,7 +33,7 @@ static IRepositoryCreationViewModel GetMeAViewModel(
         var avatarProvider = provider.GetAvatarProvider();
         var connection = provider.GetConnection();
 
-        return new RepositoryCreationViewModel(repositoryHost, os, creationService, avatarProvider);
+        return new RepositoryCreationViewModel(repositoryHost, os, creationService);
     }
 
     public class TheSafeRepositoryNameProperty : TestBaseClass
@@ -333,8 +333,7 @@ public void IsPopulatedByTheRepositoryHost()
             var vm = new RepositoryCreationViewModel(
                 repositoryHost,
                 Substitute.For<IOperatingSystem>(),
-                Substitute.For<IRepositoryCreationService>(),
-                Substitute.For<IAvatarProvider>());
+                Substitute.For<IRepositoryCreationService>());
 
             Assert.Equal(vm.Accounts[0], vm.SelectedAccount);
             Assert.Equal(2, vm.Accounts.Count);

Original file line number	Diff line number	Diff line change
`@@ -4,15 +4,15 @@`
`4`	`4`	`using System;`
`5`	`5`	`using System.Collections.Generic;`
`6`	`6`	`using System.ComponentModel.Composition;`
`7`		`-using System.Reactive.Disposables;`
	`7`	`+using System.Reactive.Linq;`
	`8`	`+using System.Threading.Tasks;`
`8`	`9`
`9`	`10`	`namespace GitHub.Factories`
`10`	`11`	`{`
`11`	`12`	`[Export(typeof(IBlobCacheFactory))]`
`12`	`13`	`[PartCreationPolicy(CreationPolicy.Shared)]`
`13`	`14`	`public class SqlitePersistentBlobCacheFactory : IBlobCacheFactory`
`14`	`15`	`{`
`15`		`- readonly CompositeDisposable disposables = new CompositeDisposable();`
`16`	`16`	`static readonly Logger log = LogManager.GetCurrentClassLogger();`
`17`	`17`	`Dictionary<string, IBlobCache> cache = new Dictionary<string, IBlobCache>();`
`18`	`18`
`@@ -26,7 +26,6 @@ public IBlobCache CreateBlobCache(string path)`
`26`	`26`	`{`
`27`	`27`	`var c = new SQLitePersistentBlobCache(path);`
`28`	`28`	`cache.Add(path, c);`
`29`		`- disposables.Add(c);`
`30`	`29`	`return c;`
`31`	`30`	`}`
`32`	`31`	`catch(Exception ex)`
`@@ -43,7 +42,14 @@ protected virtual void Dispose(bool disposing)`
`43`	`42`	`{`
`44`	`43`	`if (disposed) return;`
`45`	`44`	`disposed = true;`
`46`		`- disposables.Dispose();`
	`45`	`+ Task.Run(() =>`
	`46`	`+ {`
	`47`	`+ foreach (var c in cache.Values)`
	`48`	`+ {`
	`49`	`+ c.Dispose();`
	`50`	`+ c.Shutdown.Wait();`
	`51`	`+ }`
	`52`	`+ }).Wait(500);`
`47`	`53`	`}`
`48`	`54`	`}`
`49`	`55`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,16 @@ public IUIPair CreateViewAndViewModel(UIViewType viewType)`
`35`	`35`	`{`
`36`	`36`	`return new UIPair(viewType, factory.GetView(viewType), factory.GetViewModel(viewType));`
`37`	`37`	`}`
	`38`	`+`
	`39`	`+ protected virtual void Dispose(bool disposing)`
	`40`	`+ {}`
	`41`	`+`
	`42`	`+ public void Dispose()`
	`43`	`+ {`
	`44`	`+ Dispose(true);`
	`45`	`+ GC.SuppressFinalize(this);`
	`46`	`+ }`
	`47`	`+`
`38`	`48`	`}`
`39`	`49`
`40`	`50`	`/// <summary>`