From 53e179f68e7cb87b967cd9667ce788ec1c4d30dc Mon Sep 17 00:00:00 2001 From: Nate Harris Date: Tue, 17 Sep 2024 14:37:18 -0600 Subject: [PATCH] [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) --- CHANGELOG.md | 1 + EasyPost.Tests/ClientTest.cs | 9 ++- .../HttpTests/ClientConfigurationTest.cs | 8 ++ EasyPost.Tests/HttpTests/RequestTest.cs | 21 +++++ EasyPost.Tests/ServicesTests/ServiceTest.cs | 11 +++ .../_Utilities/Assertions/AnyException.cs | 2 +- .../Assertions/CollectionAsserts.cs | 6 -- .../Assertions/DoesNotThrowAssert.cs | 50 ++++++++++++ .../Assertions/DoesNotThrowException.cs | 19 +++++ .../Assertions/GuardArgumentNotNull.cs | 14 ++++ EasyPost/Client.cs | 76 +++++++++---------- EasyPost/ClientConfiguration.cs | 19 ++--- EasyPost/Http/Request.cs | 16 ++-- EasyPost/_base/EasyPostClient.cs | 16 ++-- EasyPost/_base/EasyPostService.cs | 15 ++-- 15 files changed, 197 insertions(+), 86 deletions(-) create mode 100644 EasyPost.Tests/HttpTests/RequestTest.cs create mode 100644 EasyPost.Tests/_Utilities/Assertions/DoesNotThrowAssert.cs create mode 100644 EasyPost.Tests/_Utilities/Assertions/DoesNotThrowException.cs create mode 100644 EasyPost.Tests/_Utilities/Assertions/GuardArgumentNotNull.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b5d2e1c5..e9fc9c731 100644 --- 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 index ece22d8ab..319f765fa 100644 --- 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 index c4409e271..c9be07561 100644 --- a/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs +++ b/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs @@ -1,6 +1,7 @@ using System; using EasyPost.Tests._Utilities.Attributes; using Xunit; +using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert; namespace EasyPost.Tests.HttpTests { @@ -8,6 +9,13 @@ 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 new file mode 100644 index 000000000..b97bcb546 --- /dev/null +++ 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 { { "key", "value" } }, new Dictionary { { "header_key", "header_value" } }); + CustomAssertions.DoesNotThrow(() => request.Dispose()); + } + + #endregion +} diff --git a/EasyPost.Tests/ServicesTests/ServiceTest.cs b/EasyPost.Tests/ServicesTests/ServiceTest.cs index 30e9f11aa..7ece449ce 100644 --- 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()); + } + /// /// 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 index 804fc7c80..ad4245a26 100644 --- a/EasyPost.Tests/_Utilities/Assertions/AnyException.cs +++ b/EasyPost.Tests/_Utilities/Assertions/AnyException.cs @@ -3,7 +3,7 @@ namespace EasyPost.Tests._Utilities.Assertions { /// - /// 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. /// public class AnyException : XunitException { diff --git a/EasyPost.Tests/_Utilities/Assertions/CollectionAsserts.cs b/EasyPost.Tests/_Utilities/Assertions/CollectionAsserts.cs index 6cbf6cf58..033774e8e 100644 --- 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); - } - /// /// 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 new file mode 100644 index 000000000..32547543e --- /dev/null +++ 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 + { + /// + /// Verifies that an action does not throw an exception. + /// + /// The action to test + /// Thrown when the action throws an exception + public static void DoesNotThrow(Action action) + { + GuardArgumentNotNull(nameof(action), action); + + try + { + action(); + } + catch (Exception ex) + { + throw new DoesNotThrowException(ex); + } + } + + /// + /// Verifies that an action does not throw a specific exception. + /// + /// The action to test + /// The type of the exception to not throw + /// Thrown when the action throws an exception of type T + public static void DoesNotThrow(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 new file mode 100644 index 000000000..6ea0588b8 --- /dev/null +++ b/EasyPost.Tests/_Utilities/Assertions/DoesNotThrowException.cs @@ -0,0 +1,19 @@ +using System; +using Xunit.Sdk; + +namespace EasyPost.Tests._Utilities.Assertions +{ + /// + /// Exception thrown when a DoesNotThrow assertion fails. + /// + public class DoesNotThrowException : XunitException + { + /// + /// Creates a new instance of the class. + /// + 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 new file mode 100644 index 000000000..42c4ba771 --- /dev/null +++ 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 index 16bbd2a61..60bcff4e8 100644 --- a/EasyPost/Client.cs +++ b/EasyPost/Client.cs @@ -190,46 +190,42 @@ public Client(ClientConfiguration configuration) /// 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 index e11b337ce..150cbd2ee 100644 --- a/EasyPost/ClientConfiguration.cs +++ b/EasyPost/ClientConfiguration.cs @@ -147,21 +147,18 @@ public void Dispose() /// 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(); } /// diff --git a/EasyPost/Http/Request.cs b/EasyPost/Http/Request.cs index 1b8812183..7054eb6a4 100644 --- a/EasyPost/Http/Request.cs +++ b/EasyPost/Http/Request.cs @@ -159,17 +159,15 @@ public void Dispose() /// 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(); } /// diff --git a/EasyPost/_base/EasyPostClient.cs b/EasyPost/_base/EasyPostClient.cs index a6db566fb..21fbb9c6a 100644 --- a/EasyPost/_base/EasyPostClient.cs +++ b/EasyPost/_base/EasyPostClient.cs @@ -222,17 +222,15 @@ public void Dispose() /// Whether this object is being disposed. 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(); } /// diff --git a/EasyPost/_base/EasyPostService.cs b/EasyPost/_base/EasyPostService.cs index ce78bd50b..9f58c741e 100644 --- a/EasyPost/_base/EasyPostService.cs +++ b/EasyPost/_base/EasyPostService.cs @@ -66,17 +66,14 @@ public void Dispose() /// protected virtual void Dispose(bool disposing) { - if (_isDisposed) return; - if (disposing) - { - // Dispose managed state (managed objects). + if (!disposing || _isDisposed) return; - // Dispose the client - Client.Dispose(); - } - - // 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) + + // Don't dispose of the associated client here, as it may be shared among multiple services } ///