[fix] Recursion causing stack overflow when disposing client/service (#…

…588) - Fix recursion causing stack overflow when disposing client/service - Add unit tests to verify disposal does not cause stack overflow - Don't dispose of client via service due to shared reference (service can be disposed of via client)
EasyPost · Sep 17, 2024 · 53e179f · 53e179f
1 parent 435251a
commit 53e179f
Show file tree

Hide file tree

Showing 15 changed files with 197 additions and 86 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## Next Release
 
 - Corrects all API documentation link references to point to their new locations
+- Fix recursion during disposal causing stack overflow
 
 ## v6.7.2 (2024-08-16)
 

diff --git a/EasyPost.Tests/ClientTest.cs b/EasyPost.Tests/ClientTest.cs
@@ -1,5 +1,4 @@
 using System;
-using System.Linq;
 using System.Net;
 using System.Net.Http;
 using System.Threading;
@@ -9,6 +8,7 @@
 using EasyPost.Tests._Utilities;
 using EasyPost.Tests._Utilities.Attributes;
 using Xunit;
+using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert;
 
 namespace EasyPost.Tests
 {
@@ -70,6 +70,13 @@ public void TestThreadSafety()
 
         private const string FakeApikey = "fake_api_key";
 
+        [Fact]
+        public void TestClientDisposal()
+        {
+            Client client = new(new ClientConfiguration(FakeApikey));
+            CustomAssertions.DoesNotThrow(() => client.Dispose());
+        }
+
         [Fact]
         public void TestBaseUrlOverride()
         {

diff --git a/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs b/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs
@@ -1,13 +1,21 @@
 using System;
 using EasyPost.Tests._Utilities.Attributes;
 using Xunit;
+using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert;
 
 namespace EasyPost.Tests.HttpTests
 {
     public class ClientConfigurationTest
     {
         #region Tests
 
+        [Fact]
+        public void TestClientConfigurationDisposal()
+        {
+            ClientConfiguration configuration = new("not_a_real_api_key");
+            CustomAssertions.DoesNotThrow(() => configuration.Dispose());
+        }
+
         [Fact]
         [Testing.Function]
         public void TestClientConfiguration()

diff --git a/EasyPost.Tests/HttpTests/RequestTest.cs b/EasyPost.Tests/HttpTests/RequestTest.cs
@@ -0,0 +1,21 @@
+using System.Collections.Generic;
+using EasyPost._base;
+using EasyPost.Http;
+using Xunit;
+using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert;
+
+namespace EasyPost.Tests.HttpTests;
+
+public class RequestTest
+{
+    #region Tests
+
+    [Fact]
+    public void TestRequestDisposal()
+    {
+        Request request = new("https://google.com", "not_a_real_endpoint", Method.Get, ApiVersion.V2, new Dictionary<string, object> { { "key", "value" } }, new Dictionary<string, string> { { "header_key", "header_value" } });
+        CustomAssertions.DoesNotThrow(() => request.Dispose());
+    }
+
+    #endregion
+}
diff --git a/EasyPost.Tests/ServicesTests/ServiceTest.cs b/EasyPost.Tests/ServicesTests/ServiceTest.cs
@@ -5,10 +5,12 @@
 using EasyPost.Exceptions.General;
 using EasyPost.Http;
 using EasyPost.Models.API;
+using EasyPost.Services;
 using EasyPost.Tests._Utilities;
 using EasyPost.Tests._Utilities.Attributes;
 using EasyPost.Utilities.Internal.Attributes;
 using Xunit;
+using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert;
 
 namespace EasyPost.Tests.ServicesTests
 {
@@ -21,6 +23,15 @@ public ServiceTests() : base("base_service")
         {
         }
 
+        [Fact]
+        public void TestServiceDisposal()
+        {
+            Client client = new(new ClientConfiguration("not_a_real_api_key"));
+
+            AddressService addressService = client.Address;
+            CustomAssertions.DoesNotThrow(() => addressService.Dispose());
+        }
+
         /// <summary>
         ///     This test confirms that the GetNextPage method works as expected.
         ///     This method is implemented on a per-service, but rather than testing in each service, we'll test it once here.

diff --git a/EasyPost.Tests/_Utilities/Assertions/AnyException.cs b/EasyPost.Tests/_Utilities/Assertions/AnyException.cs
@@ -3,7 +3,7 @@
 namespace EasyPost.Tests._Utilities.Assertions
 {
     /// <summary>
-    /// Exception thrown when an Any assertion has one or more items fail an assertion.
+    ///     Exception thrown when an Any assertion has one or more items fail an assertion.
     /// </summary>
     public class AnyException : XunitException
     {

diff --git a/EasyPost.Tests/_Utilities/Assertions/CollectionAsserts.cs b/EasyPost.Tests/_Utilities/Assertions/CollectionAsserts.cs
@@ -6,12 +6,6 @@ namespace EasyPost.Tests._Utilities.Assertions
     // ReSharper disable once PartialTypeWithSinglePart
     public abstract partial class Assert
     {
-        private static void GuardArgumentNotNull(string argName, object argValue)
-        {
-            if (argValue == null)
-                throw new ArgumentNullException(argName);
-        }
-
         /// <summary>
         /// Verifies that any items in the collection pass when executed against
         /// action.

diff --git a/EasyPost.Tests/_Utilities/Assertions/DoesNotThrowAssert.cs b/EasyPost.Tests/_Utilities/Assertions/DoesNotThrowAssert.cs
@@ -0,0 +1,50 @@
+using System;
+
+namespace EasyPost.Tests._Utilities.Assertions
+{
+    // ReSharper disable once PartialTypeWithSinglePart
+    public abstract partial class Assert
+    {
+        /// <summary>
+        ///     Verifies that an action does not throw an exception.
+        /// </summary>
+        /// <param name="action">The action to test</param>
+        /// <exception cref="DoesNotThrowException">Thrown when the action throws an exception</exception>
+        public static void DoesNotThrow(Action action)
+        {
+            GuardArgumentNotNull(nameof(action), action);
+
+            try
+            {
+                action();
+            }
+            catch (Exception ex)
+            {
+                throw new DoesNotThrowException(ex);
+            }
+        }
+
+        /// <summary>
+        ///     Verifies that an action does not throw a specific exception.
+        /// </summary>
+        /// <param name="action">The action to test</param>
+        /// <typeparam name="T">The type of the exception to not throw</typeparam>
+        /// <exception cref="DoesNotThrowException">Thrown when the action throws an exception of type T</exception>
+        public static void DoesNotThrow<T>(Action action) where T : Exception
+        {
+            GuardArgumentNotNull(nameof(action), action);
+
+            try
+            {
+                action();
+            }
+            catch (Exception ex)
+            {
+                if (ex.GetType() == typeof(T))
+                {
+                    throw new DoesNotThrowException(ex);
+                }
+            }
+        }
+    }
+}
diff --git a/EasyPost.Tests/_Utilities/Assertions/DoesNotThrowException.cs b/EasyPost.Tests/_Utilities/Assertions/DoesNotThrowException.cs
@@ -0,0 +1,19 @@
+using System;
+using Xunit.Sdk;
+
+namespace EasyPost.Tests._Utilities.Assertions
+{
+    /// <summary>
+    ///     Exception thrown when a DoesNotThrow assertion fails.
+    /// </summary>
+    public class DoesNotThrowException : XunitException
+    {
+        /// <summary>
+        ///     Creates a new instance of the <see cref="DoesNotThrowException"/> class.
+        /// </summary>
+        public DoesNotThrowException(Exception ex)
+            : base("Assert.DoesNotThrow() Failure", ex)
+        {
+        }
+    }
+}
diff --git a/EasyPost.Tests/_Utilities/Assertions/GuardArgumentNotNull.cs b/EasyPost.Tests/_Utilities/Assertions/GuardArgumentNotNull.cs
@@ -0,0 +1,14 @@
+using System;
+
+namespace EasyPost.Tests._Utilities.Assertions
+{
+    // ReSharper disable once PartialTypeWithSinglePart
+    public abstract partial class Assert
+    {
+        private static void GuardArgumentNotNull(string argName, object argValue)
+        {
+            if (argValue == null)
+                throw new ArgumentNullException(argName);
+        }
+    }
+}
diff --git a/EasyPost/Client.cs b/EasyPost/Client.cs
@@ -190,46 +190,42 @@ public Client(ClientConfiguration configuration)
         /// <inheritdoc cref="EasyPostClient.Dispose(bool)"/>
         protected override void Dispose(bool disposing)
         {
-            // ref: https://dzone.com/articles/when-and-how-to-use-dispose-and-finalize-in-c
-            // ref: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1063#pseudo-code-example
-            if (disposing)
-            {
-                // Dispose managed state (managed objects).
-                // "disposing" inherently true when called from Dispose(), so don't need to pass it in.
-
-                // Dispose of the services
-                Address.Dispose();
-                ApiKey.Dispose();
-                Batch.Dispose();
-                Billing.Dispose();
-                CarrierAccount.Dispose();
-                CarrierMetadata.Dispose();
-                CarrierType.Dispose();
-                Claim.Dispose();
-                CustomsInfo.Dispose();
-                CustomsItem.Dispose();
-                EndShipper.Dispose();
-                Event.Dispose();
-                Insurance.Dispose();
-                Order.Dispose();
-                Parcel.Dispose();
-                Pickup.Dispose();
-                Rate.Dispose();
-                ReferralCustomer.Dispose();
-                Refund.Dispose();
-                Report.Dispose();
-                ScanForm.Dispose();
-                Shipment.Dispose();
-                SmartRate.Dispose();
-                Tracker.Dispose();
-                User.Dispose();
-                Webhook.Dispose();
-
-                // Dispose of the Beta client
-                Beta.Dispose();
-            }
-
-            // Free native resources (unmanaged objects) and override a finalizer below.
+            if (!disposing) return;
+
+            // Dispose managed state (managed objects)
+
+            // "disposing" inherently true when called from Dispose(), so don't need to pass it in.
+
+            // Attempt to dispose of the services (some may already be disposed)
+            Address.Dispose();
+            ApiKey.Dispose();
+            Batch.Dispose();
+            Billing.Dispose();
+            CarrierAccount.Dispose();
+            CarrierMetadata.Dispose();
+            CarrierType.Dispose();
+            Claim.Dispose();
+            CustomsInfo.Dispose();
+            CustomsItem.Dispose();
+            EndShipper.Dispose();
+            Event.Dispose();
+            Insurance.Dispose();
+            Order.Dispose();
+            Parcel.Dispose();
+            Pickup.Dispose();
+            Rate.Dispose();
+            ReferralCustomer.Dispose();
+            Refund.Dispose();
+            Report.Dispose();
+            ScanForm.Dispose();
+            Shipment.Dispose();
+            SmartRate.Dispose();
+            Tracker.Dispose();
+            User.Dispose();
+            Webhook.Dispose();
+
+            // Attempt to dispose of the Beta client (may already be disposed)
+            Beta.Dispose();
 
             // Dispose of the base client
             base.Dispose(disposing);

diff --git a/EasyPost/ClientConfiguration.cs b/EasyPost/ClientConfiguration.cs
@@ -147,21 +147,18 @@ public void Dispose()
     /// <inheritdoc cref="EasyPostClient.Dispose(bool)"/>
     protected virtual void Dispose(bool disposing)
     {
-        if (_isDisposed) return;
+        if (!disposing || _isDisposed) return;
 
-        if (disposing)
-        {
-            // Dispose managed state (managed objects).
+        // Set the disposed flag to true before disposing of the object to avoid infinite loops
+        _isDisposed = true;
 
-            // dispose of the prepared HTTP client
-            PreparedHttpClient?.Dispose();
+        // Dispose managed state (managed objects)
 
-            // dispose of the user-provided HTTP client
-            CustomHttpClient?.Dispose();
-        }
+        // Attempt to dispose of the prepared HTTP client (may already be disposed)
+        PreparedHttpClient?.Dispose();
 
-        // Free native resources (unmanaged objects) and override a finalizer below.
-        _isDisposed = true;
+        // Attempt to dispose of the user-provided HTTP client (may already be disposed)
+        CustomHttpClient?.Dispose();
     }
 
     /// <summary>

diff --git a/EasyPost/Http/Request.cs b/EasyPost/Http/Request.cs
@@ -159,17 +159,15 @@ public void Dispose()
         /// <inheritdoc cref="EasyPostClient.Dispose(bool)"/>
         protected virtual void Dispose(bool disposing)
         {
-            if (_isDisposed) return;
-            if (disposing)
-            {
-                // Dispose managed state (managed objects).
-
-                // Dispose the request message
-                _requestMessage.Dispose();
-            }
+            if (!disposing || _isDisposed) return;
 
-            // Free native resources (unmanaged objects) and override a finalizer below.
+            // Set the disposed flag to true before disposing of the object to avoid infinite loops
             _isDisposed = true;
+
+            // Dispose managed state (managed objects)
+
+            // Dispose the request message
+            _requestMessage.Dispose();
         }
 
         /// <summary>

diff --git a/EasyPost/_base/EasyPostClient.cs b/EasyPost/_base/EasyPostClient.cs
@@ -222,17 +222,15 @@ public void Dispose()
         /// <param name="disposing">Whether this object is being disposed.</param>
         protected virtual void Dispose(bool disposing)
         {
-            if (_isDisposed) return;
-            if (disposing)
-            {
-                // Dispose managed state (managed objects).
-
-                // Dispose the configuration
-                _configuration.Dispose();
-            }
+            if (!disposing || _isDisposed) return;
 
-            // Free native resources (unmanaged objects) and override a finalizer below.
+            // Set the disposed flag to true before disposing of the object to avoid infinite loops
             _isDisposed = true;
+
+            // Dispose managed state (managed objects)
+
+            // Dispose of the configuration
+            _configuration.Dispose();
         }
 
         /// <summary>