Introduce DisposableObject base class to impl the dispose pattern

mjcheetham · mjcheetham · commit f835d1390ab1 · 2019-09-04T11:19:33.000+01:00
Implement the dispose pattern correctly for all IDisposable objects by
introducing a common abstract `DisposableObject` class which 'does the
correct thing'.
diff --git a/src/shared/GitHub/GitHubHostProvider.cs b/src/shared/GitHub/GitHubHostProvider.cs
@@ -64,6 +64,8 @@ public override string GetCredentialKey(InputArguments input)
 
         public override async Task<ICredential> GenerateCredentialAsync(InputArguments input)
         {
+            ThrowIfDisposed();
+
             // We should not allow unencrypted communication and should inform the user
             if (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http"))
             {
@@ -105,14 +107,10 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i
             throw new Exception($"Interactive logon for '{targetUri}' failed.");
         }
 
-        protected override void Dispose(bool disposing)
+        protected override void ReleaseManagedResources()
         {
-            if (disposing)
-            {
-                _gitHubApi.Dispose();
-            }
-
-            base.Dispose(disposing);
+            _gitHubApi.Dispose();
+            base.ReleaseManagedResources();
         }
 
         #region Private Methods
diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs
@@ -54,6 +54,8 @@ public override string GetCredentialKey(InputArguments input)
 
         public override async Task<ICredential> GenerateCredentialAsync(InputArguments input)
         {
+            ThrowIfDisposed();
+
             // We should not allow unencrypted communication and should inform the user
             if (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http"))
             {
@@ -94,14 +96,10 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i
             return new GitCredential(Constants.PersonalAccessTokenUserName, pat);
         }
 
-        protected override void Dispose(bool disposing)
+        protected override void ReleaseManagedResources()
         {
-            if (disposing)
-            {
-                _azDevOps.Dispose();
-            }
-
-            base.Dispose(disposing);
+            _azDevOps.Dispose();
+            base.ReleaseManagedResources();
         }
 
         #endregion
diff --git a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs
@@ -54,7 +54,7 @@ public interface ICommandContext : IDisposable
     /// <summary>
     /// Real command execution environment using the actual <see cref="Console"/>, file system calls and environment.
     /// </summary>
-    public class CommandContext : ICommandContext
+    public class CommandContext : DisposableObject, ICommandContext
     {
         private readonly IGit _git;
 
@@ -111,10 +111,13 @@ public CommandContext()
 
         #region IDisposable
 
-        public void Dispose()
+        protected override void ReleaseManagedResources()
         {
             Settings?.Dispose();
             _git?.Dispose();
+            Trace?.Dispose();
+
+            base.ReleaseManagedResources();
         }
 
         #endregion
diff --git a/src/shared/Microsoft.Git.CredentialManager/DisposableObject.cs b/src/shared/Microsoft.Git.CredentialManager/DisposableObject.cs
@@ -0,0 +1,68 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license.
+using System;
+
+namespace Microsoft.Git.CredentialManager
+{
+    /// <summary>
+    /// An object that implements the <see cref="IDisposable"/> interface and the disposable pattern.
+    /// </summary>
+    public abstract class DisposableObject : IDisposable
+    {
+        private bool _isDisposed;
+
+        /// <summary>
+        /// Throw an exception if the object has been disposed.
+        /// </summary>
+        /// <exception cref="ObjectDisposedException">Thrown if the object has been disposed.</exception>
+        protected void ThrowIfDisposed()
+        {
+            if (_isDisposed)
+            {
+                throw new ObjectDisposedException(GetType().Name);
+            }
+        }
+
+        /// <summary>
+        /// Called when unmanaged resources should be released and memory freed.
+        /// </summary>
+        protected virtual void ReleaseUnmanagedResources() { }
+
+        /// <summary>
+        /// Called when managed resources should be released.
+        /// </summary>
+        protected virtual void ReleaseManagedResources() { }
+
+        /// <summary>
+        /// Called when the application is being terminated. Clean up and release any resources.
+        /// </summary>
+        /// <param name="disposing">True if the instance is being disposed, false if being finalized.</param>
+        private void Dispose(bool disposing)
+        {
+            if (_isDisposed)
+            {
+                return;
+            }
+
+            ReleaseUnmanagedResources();
+
+            if (disposing)
+            {
+                ReleaseManagedResources();
+            }
+
+            _isDisposed = true;
+        }
+
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        ~DisposableObject()
+        {
+            Dispose(false);
+        }
+    }
+}
diff --git a/src/shared/Microsoft.Git.CredentialManager/GenericHostProvider.cs b/src/shared/Microsoft.Git.CredentialManager/GenericHostProvider.cs
@@ -53,6 +53,8 @@ public override string GetCredentialKey(InputArguments input)
 
         public override async Task<ICredential> GenerateCredentialAsync(InputArguments input)
         {
+            ThrowIfDisposed();
+
             Uri uri = GetUriFromInput(input);
 
             // Determine the if the host supports Windows Integration Authentication (WIA)
@@ -115,10 +117,10 @@ private bool IsWindowsAuthAllowed
             }
         }
 
-        protected override void Dispose(bool disposing)
+        protected override void ReleaseManagedResources()
         {
             _winAuth.Dispose();
-            base.Dispose(disposing);
+            base.ReleaseManagedResources();
         }
 
         #endregion
diff --git a/src/shared/Microsoft.Git.CredentialManager/HostProvider.cs b/src/shared/Microsoft.Git.CredentialManager/HostProvider.cs
@@ -58,7 +58,7 @@ public interface IHostProvider : IDisposable
     /// Represents a Git hosting provider where credentials can be stored and recalled in/from the Operating System's
     /// secure credential store.
     /// </summary>
-    public abstract class HostProvider : IHostProvider
+    public abstract class HostProvider : DisposableObject, IHostProvider
     {
         protected HostProvider(ICommandContext context)
         {
@@ -194,22 +194,5 @@ public virtual Task EraseCredentialAsync(InputArguments input)
 
             return Task.CompletedTask;
         }
-
-        /// <summary>
-        /// Called when the application is being terminated. Clean up and release any resources.
-        /// </summary>
-        /// <param name="disposing">True if the instance is being disposed, false if being finalized.</param>
-        protected virtual void Dispose(bool disposing) { }
-
-        public void Dispose()
-        {
-            Dispose(true);
-            GC.SuppressFinalize(this);
-        }
-
-        ~HostProvider()
-        {
-            Dispose(false);
-        }
     }
 }
diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/LibGit2.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/LibGit2.cs
@@ -8,7 +8,7 @@
 
 namespace Microsoft.Git.CredentialManager.Interop
 {
-    public class LibGit2 : IGit
+    public class LibGit2 : DisposableObject, IGit
     {
         private readonly ITrace _trace;
 
@@ -24,6 +24,8 @@ public LibGit2(ITrace trace)
 
         public unsafe IGitConfiguration GetConfiguration(string repositoryPath)
         {
+            ThrowIfDisposed();
+
             _trace.WriteLine("Opening default Git configuration...");
 
             // Open the default, non-repository-scoped configuration (progdata, system, xdg, global)
@@ -59,6 +61,8 @@ public unsafe IGitConfiguration GetConfiguration(string repositoryPath)
 
         public string GetRepositoryPath(string path)
         {
+            ThrowIfDisposed();
+
             _trace.WriteLine($"Discovering repository from path '{path}'...");
 
             var buf = new git_buf();
@@ -85,24 +89,14 @@ public string GetRepositoryPath(string path)
             }
         }
 
-        private void Dispose(bool disposing)
+        protected override void ReleaseUnmanagedResources()
         {
             git_libgit2_shutdown();
-        }
-
-        public void Dispose()
-        {
-            Dispose(true);
-            GC.SuppressFinalize(this);
-        }
-
-        ~LibGit2()
-        {
-            Dispose(false);
+            base.ReleaseUnmanagedResources();
         }
     }
 
-    internal class LibGit2Configuration : IGitConfiguration
+    internal class LibGit2Configuration : DisposableObject, IGitConfiguration
     {
         private readonly ITrace _trace;
         private readonly unsafe git_config* _config;
@@ -122,6 +116,8 @@ internal unsafe LibGit2Configuration(ITrace trace, git_config* config)
 
         public unsafe void Enumerate(GitConfigurationEnumerationCallback cb)
         {
+            ThrowIfDisposed();
+
             int native_cb(git_config_entry entry, void* payload)
             {
                 if (!cb(entry.GetName(), entry.GetValue()))
@@ -149,6 +145,8 @@ int native_cb(git_config_entry entry, void* payload)
 
         public unsafe bool TryGetValue(string name, out string value)
         {
+            ThrowIfDisposed();
+
             _trace.WriteLine($"Reading Git configuration entry '{name}'...");
             int result = git_config_get_string(out value, _snapshot, name);
 
@@ -169,7 +167,7 @@ public unsafe bool TryGetValue(string name, out string value)
             return false;
         }
 
-        private void Dispose(bool disposing)
+        protected override void ReleaseUnmanagedResources()
         {
             unsafe
             {
@@ -178,16 +176,5 @@ private void Dispose(bool disposing)
                 git_config_free(_config);
             }
         }
-
-        public void Dispose()
-        {
-            Dispose(true);
-            GC.SuppressFinalize(this);
-        }
-
-        ~LibGit2Configuration()
-        {
-            Dispose(false);
-        }
     }
 }
diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Posix/PosixFileDescriptor.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Posix/PosixFileDescriptor.cs
@@ -9,12 +9,10 @@ namespace Microsoft.Git.CredentialManager.Interop.Posix
     /// <summary>
     /// Represents a thin wrapper over a POSIX file descriptor.
     /// </summary>
-    public class PosixFileDescriptor : IDisposable
+    public class PosixFileDescriptor : DisposableObject
     {
         private readonly int _fd;
 
-        private bool _isDisposed;
-
         private PosixFileDescriptor()
         {
             PlatformUtils.EnsurePosix();
@@ -72,27 +70,14 @@ public int Write(string str)
             return Write(buf, buf.Length);
         }
 
-        private void Dispose(bool disposing)
+        protected override void ReleaseUnmanagedResources()
         {
-            if (_isDisposed)
-            {
-                return;
-            }
-
             if (!IsInvalid)
             {
                 Unistd.close(_fd);
             }
 
-            _isDisposed = true;
-        }
-
-        private void ThrowIfDisposed()
-        {
-            if (_isDisposed)
-            {
-                throw new ObjectDisposedException(nameof(PosixFileDescriptor));
-            }
+            base.ReleaseUnmanagedResources();
         }
 
         private void ThrowIfInvalid()
@@ -102,16 +87,5 @@ private void ThrowIfInvalid()
                 throw new InvalidOperationException("File descriptor is invalid");
             }
         }
-
-        public void Dispose()
-        {
-            Dispose(true);
-            GC.SuppressFinalize(this);
-        }
-
-        ~PosixFileDescriptor()
-        {
-            Dispose(false);
-        }
     }
 }
diff --git a/src/shared/Microsoft.Git.CredentialManager/Trace.cs b/src/shared/Microsoft.Git.CredentialManager/Trace.cs
diff --git a/src/shared/TestInfrastructure/Objects/NullTrace.cs b/src/shared/TestInfrastructure/Objects/NullTrace.cs

Original file line number	Diff line number	Diff line change
`@@ -64,6 +64,8 @@ public override string GetCredentialKey(InputArguments input)`
`64`	`64`
`65`	`65`	`public override async Task<ICredential> GenerateCredentialAsync(InputArguments input)`
`66`	`66`	`{`
	`67`	`+ ThrowIfDisposed();`
	`68`	`+`
`67`	`69`	`// We should not allow unencrypted communication and should inform the user`
`68`	`70`	`if (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http"))`
`69`	`71`	`{`
`@@ -105,14 +107,10 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i`
`105`	`107`	`throw new Exception($"Interactive logon for '{targetUri}' failed.");`
`106`	`108`	`}`
`107`	`109`
`108`		`- protected override void Dispose(bool disposing)`
	`110`	`+ protected override void ReleaseManagedResources()`
`109`	`111`	`{`
`110`		`- if (disposing)`
`111`		`- {`
`112`		`- _gitHubApi.Dispose();`
`113`		`- }`
`114`		`-`
`115`		`- base.Dispose(disposing);`
	`112`	`+ _gitHubApi.Dispose();`
	`113`	`+ base.ReleaseManagedResources();`
`116`	`114`	`}`
`117`	`115`
`118`	`116`	`#region Private Methods`
Original file line number	Diff line number	Diff line change
`@@ -54,6 +54,8 @@ public override string GetCredentialKey(InputArguments input)`
`54`	`54`
`55`	`55`	`public override async Task<ICredential> GenerateCredentialAsync(InputArguments input)`
`56`	`56`	`{`
	`57`	`+ ThrowIfDisposed();`
	`58`	`+`
`57`	`59`	`// We should not allow unencrypted communication and should inform the user`
`58`	`60`	`if (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http"))`
`59`	`61`	`{`
`@@ -94,14 +96,10 @@ public override async Task<ICredential> GenerateCredentialAsync(InputArguments i`
`94`	`96`	`return new GitCredential(Constants.PersonalAccessTokenUserName, pat);`
`95`	`97`	`}`
`96`	`98`
`97`		`- protected override void Dispose(bool disposing)`
	`99`	`+ protected override void ReleaseManagedResources()`
`98`	`100`	`{`
`99`		`- if (disposing)`
`100`		`- {`
`101`		`- _azDevOps.Dispose();`
`102`		`- }`
`103`		`-`
`104`		`- base.Dispose(disposing);`
	`101`	`+ _azDevOps.Dispose();`
	`102`	`+ base.ReleaseManagedResources();`
`105`	`103`	`}`
`106`	`104`
`107`	`105`	`#endregion`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ public interface ICommandContext : IDisposable`
`54`	`54`	`/// <summary>`
`55`	`55`	`/// Real command execution environment using the actual <see cref="Console"/>, file system calls and environment.`
`56`	`56`	`/// </summary>`
`57`		`- public class CommandContext : ICommandContext`
	`57`	`+ public class CommandContext : DisposableObject, ICommandContext`
`58`	`58`	`{`
`59`	`59`	`private readonly IGit _git;`
`60`	`60`
`@@ -111,10 +111,13 @@ public CommandContext()`
`111`	`111`
`112`	`112`	`#region IDisposable`
`113`	`113`
`114`		`- public void Dispose()`
	`114`	`+ protected override void ReleaseManagedResources()`
`115`	`115`	`{`
`116`	`116`	`Settings?.Dispose();`
`117`	`117`	`_git?.Dispose();`
	`118`	`+ Trace?.Dispose();`
	`119`	`+`
	`120`	`+ base.ReleaseManagedResources();`
`118`	`121`	`}`
`119`	`122`
`120`	`123`	`#endregion`
Original file line number	Diff line number	Diff line change
`@@ -53,6 +53,8 @@ public override string GetCredentialKey(InputArguments input)`
`53`	`53`
`54`	`54`	`public override async Task<ICredential> GenerateCredentialAsync(InputArguments input)`
`55`	`55`	`{`
	`56`	`+ ThrowIfDisposed();`
	`57`	`+`
`56`	`58`	`Uri uri = GetUriFromInput(input);`
`57`	`59`
`58`	`60`	`// Determine the if the host supports Windows Integration Authentication (WIA)`
`@@ -115,10 +117,10 @@ private bool IsWindowsAuthAllowed`
`115`	`117`	`}`
`116`	`118`	`}`
`117`	`119`
`118`		`- protected override void Dispose(bool disposing)`
	`120`	`+ protected override void ReleaseManagedResources()`
`119`	`121`	`{`
`120`	`122`	`_winAuth.Dispose();`
`121`		`- base.Dispose(disposing);`
	`123`	`+ base.ReleaseManagedResources();`
`122`	`124`	`}`
`123`	`125`
`124`	`126`	`#endregion`
Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@`
`8`	`8`
`9`	`9`	`namespace Microsoft.Git.CredentialManager.Interop`
`10`	`10`	`{`
`11`		`- public class LibGit2 : IGit`
	`11`	`+ public class LibGit2 : DisposableObject, IGit`
`12`	`12`	`{`
`13`	`13`	`private readonly ITrace _trace;`
`14`	`14`
`@@ -24,6 +24,8 @@ public LibGit2(ITrace trace)`
`24`	`24`
`25`	`25`	`public unsafe IGitConfiguration GetConfiguration(string repositoryPath)`
`26`	`26`	`{`
	`27`	`+ ThrowIfDisposed();`
	`28`	`+`
`27`	`29`	`_trace.WriteLine("Opening default Git configuration...");`
`28`	`30`
`29`	`31`	`// Open the default, non-repository-scoped configuration (progdata, system, xdg, global)`
`@@ -59,6 +61,8 @@ public unsafe IGitConfiguration GetConfiguration(string repositoryPath)`
`59`	`61`
`60`	`62`	`public string GetRepositoryPath(string path)`
`61`	`63`	`{`
	`64`	`+ ThrowIfDisposed();`
	`65`	`+`
`62`	`66`	`_trace.WriteLine($"Discovering repository from path '{path}'...");`
`63`	`67`
`64`	`68`	`var buf = new git_buf();`
`@@ -85,24 +89,14 @@ public string GetRepositoryPath(string path)`
`85`	`89`	`}`
`86`	`90`	`}`
`87`	`91`
`88`		`- private void Dispose(bool disposing)`
	`92`	`+ protected override void ReleaseUnmanagedResources()`
`89`	`93`	`{`
`90`	`94`	`git_libgit2_shutdown();`
`91`		`- }`
`92`		`-`
`93`		`- public void Dispose()`
`94`		`- {`
`95`		`- Dispose(true);`
`96`		`- GC.SuppressFinalize(this);`
`97`		`- }`
`98`		`-`
`99`		`- ~LibGit2()`
`100`		`- {`
`101`		`- Dispose(false);`
	`95`	`+ base.ReleaseUnmanagedResources();`
`102`	`96`	`}`
`103`	`97`	`}`
`104`	`98`
`105`		`- internal class LibGit2Configuration : IGitConfiguration`
	`99`	`+ internal class LibGit2Configuration : DisposableObject, IGitConfiguration`
`106`	`100`	`{`
`107`	`101`	`private readonly ITrace _trace;`
`108`	`102`	`private readonly unsafe git_config* _config;`
`@@ -122,6 +116,8 @@ internal unsafe LibGit2Configuration(ITrace trace, git_config* config)`
`122`	`116`
`123`	`117`	`public unsafe void Enumerate(GitConfigurationEnumerationCallback cb)`
`124`	`118`	`{`
	`119`	`+ ThrowIfDisposed();`
	`120`	`+`
`125`	`121`	`int native_cb(git_config_entry entry, void* payload)`
`126`	`122`	`{`
`127`	`123`	`if (!cb(entry.GetName(), entry.GetValue()))`
`@@ -149,6 +145,8 @@ int native_cb(git_config_entry entry, void* payload)`
`149`	`145`
`150`	`146`	`public unsafe bool TryGetValue(string name, out string value)`
`151`	`147`	`{`
	`148`	`+ ThrowIfDisposed();`
	`149`	`+`
`152`	`150`	`_trace.WriteLine($"Reading Git configuration entry '{name}'...");`
`153`	`151`	`int result = git_config_get_string(out value, _snapshot, name);`
`154`	`152`
`@@ -169,7 +167,7 @@ public unsafe bool TryGetValue(string name, out string value)`
`169`	`167`	`return false;`
`170`	`168`	`}`
`171`	`169`
`172`		`- private void Dispose(bool disposing)`
	`170`	`+ protected override void ReleaseUnmanagedResources()`
`173`	`171`	`{`
`174`	`172`	`unsafe`
`175`	`173`	`{`
`@@ -178,16 +176,5 @@ private void Dispose(bool disposing)`
`178`	`176`	`git_config_free(_config);`
`179`	`177`	`}`
`180`	`178`	`}`
`181`		`-`
`182`		`- public void Dispose()`
`183`		`- {`
`184`		`- Dispose(true);`
`185`		`- GC.SuppressFinalize(this);`
`186`		`- }`
`187`		`-`
`188`		`- ~LibGit2Configuration()`
`189`		`- {`
`190`		`- Dispose(false);`
`191`		`- }`
`192`	`179`	`}`
`193`	`180`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,12 +9,10 @@ namespace Microsoft.Git.CredentialManager.Interop.Posix`
`9`	`9`	`/// <summary>`
`10`	`10`	`/// Represents a thin wrapper over a POSIX file descriptor.`
`11`	`11`	`/// </summary>`
`12`		`- public class PosixFileDescriptor : IDisposable`
	`12`	`+ public class PosixFileDescriptor : DisposableObject`
`13`	`13`	`{`
`14`	`14`	`private readonly int _fd;`
`15`	`15`
`16`		`- private bool _isDisposed;`
`17`		`-`
`18`	`16`	`private PosixFileDescriptor()`
`19`	`17`	`{`
`20`	`18`	`PlatformUtils.EnsurePosix();`
`@@ -72,27 +70,14 @@ public int Write(string str)`
`72`	`70`	`return Write(buf, buf.Length);`
`73`	`71`	`}`
`74`	`72`
`75`		`- private void Dispose(bool disposing)`
	`73`	`+ protected override void ReleaseUnmanagedResources()`
`76`	`74`	`{`
`77`		`- if (_isDisposed)`
`78`		`- {`
`79`		`- return;`
`80`		`- }`
`81`		`-`
`82`	`75`	`if (!IsInvalid)`
`83`	`76`	`{`
`84`	`77`	`Unistd.close(_fd);`
`85`	`78`	`}`
`86`	`79`
`87`		`- _isDisposed = true;`
`88`		`- }`
`89`		`-`
`90`		`- private void ThrowIfDisposed()`
`91`		`- {`
`92`		`- if (_isDisposed)`
`93`		`- {`
`94`		`- throw new ObjectDisposedException(nameof(PosixFileDescriptor));`
`95`		`- }`
	`80`	`+ base.ReleaseUnmanagedResources();`
`96`	`81`	`}`
`97`	`82`
`98`	`83`	`private void ThrowIfInvalid()`
`@@ -102,16 +87,5 @@ private void ThrowIfInvalid()`
`102`	`87`	`throw new InvalidOperationException("File descriptor is invalid");`
`103`	`88`	`}`
`104`	`89`	`}`
`105`		`-`
`106`		`- public void Dispose()`
`107`		`- {`
`108`		`- Dispose(true);`
`109`		`- GC.SuppressFinalize(this);`
`110`		`- }`
`111`		`-`
`112`		`- ~PosixFileDescriptor()`
`113`		`- {`
`114`		`- Dispose(false);`
`115`		`- }`
`116`	`90`	`}`
`117`	`91`	`}`