From ab3d1c0adb73894e52e5e7bfcf334221f55be0da Mon Sep 17 00:00:00 2001 From: Nate Harris Date: Tue, 17 Sep 2024 13:26:38 -0600 Subject: [PATCH 1/5] - Fix recursion causing stack overflow when disposing client/service - Add unit tests to verify disposal does not cause stack overflow Closes #587 --- EasyPost.Tests/ClientTest.cs | 9 +++ .../HttpTests/ClientConfigurationTest.cs | 10 +++ EasyPost.Tests/HttpTests/RequestTest.cs | 23 ++++++ EasyPost.Tests/ServicesTests/ServiceTest.cs | 12 +++ EasyPost/Client.cs | 76 +++++++++---------- EasyPost/ClientConfiguration.cs | 19 ++--- EasyPost/Http/Request.cs | 16 ++-- EasyPost/_base/EasyPostClient.cs | 16 ++-- EasyPost/_base/EasyPostService.cs | 16 ++-- 9 files changed, 119 insertions(+), 78 deletions(-) create mode 100644 EasyPost.Tests/HttpTests/RequestTest.cs diff --git a/EasyPost.Tests/ClientTest.cs b/EasyPost.Tests/ClientTest.cs index ece22d8ab..0e803a086 100644 --- a/EasyPost.Tests/ClientTest.cs +++ b/EasyPost.Tests/ClientTest.cs @@ -70,6 +70,15 @@ public void TestThreadSafety() private const string FakeApikey = "fake_api_key"; + [Fact] + public void TestClientDisposal() + { + Client client = new(new ClientConfiguration(FakeApikey)); + client.Dispose(); + + // As long as this test doesn't throw an exception, it passes + } + [Fact] public void TestBaseUrlOverride() { diff --git a/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs b/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs index c4409e271..69ee78201 100644 --- a/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs +++ b/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs @@ -8,6 +8,16 @@ public class ClientConfigurationTest { #region Tests + [Fact] + public void TestClientConfigurationDisposal() + { + ClientConfiguration configuration = new("not_a_real_api_key"); + + configuration.Dispose(); + + // As long as this test doesn't throw an exception, it passes + } + [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..edd01ccb6 --- /dev/null +++ b/EasyPost.Tests/HttpTests/RequestTest.cs @@ -0,0 +1,23 @@ +using System.Collections.Generic; +using EasyPost._base; +using EasyPost.Http; +using Xunit; + +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" } }); + + request.Dispose(); + + // As long as this test doesn't throw an exception, it passes + } + + #endregion +} diff --git a/EasyPost.Tests/ServicesTests/ServiceTest.cs b/EasyPost.Tests/ServicesTests/ServiceTest.cs index 30e9f11aa..eb24369ea 100644 --- a/EasyPost.Tests/ServicesTests/ServiceTest.cs +++ b/EasyPost.Tests/ServicesTests/ServiceTest.cs @@ -5,6 +5,7 @@ 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; @@ -21,6 +22,17 @@ public ServiceTests() : base("base_service") { } + [Fact] + public void TestServiceDisposal() + { + Client client = new(new ClientConfiguration("not_a_real_api_key")); + + AddressService addressService = client.Address; + addressService.Dispose(); // Will also dispose the underlying client + + // As long as this test doesn't throw an exception, it passes + } + /// /// 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/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..61d9e6d09 100644 --- a/EasyPost/_base/EasyPostService.cs +++ b/EasyPost/_base/EasyPostService.cs @@ -66,17 +66,15 @@ 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) + + // Attempt to dispose the client (may already be disposed) + Client.Dispose(); } /// From c07e1318384719cd5ca6c4cf8eea1fb0d56ef282 Mon Sep 17 00:00:00 2001 From: Nate Harris Date: Tue, 17 Sep 2024 13:28:36 -0600 Subject: [PATCH 2/5] - Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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) From 0aa9bf4ef329f72de7e2e0aa1da51a6a6d7028c9 Mon Sep 17 00:00:00 2001 From: Nate Harris Date: Tue, 17 Sep 2024 13:31:34 -0600 Subject: [PATCH 3/5] - Don't dispose of client via service due to shared reference (service can be diposed of via client) --- EasyPost.Tests/ServicesTests/ServiceTest.cs | 2 +- EasyPost/_base/EasyPostService.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/EasyPost.Tests/ServicesTests/ServiceTest.cs b/EasyPost.Tests/ServicesTests/ServiceTest.cs index eb24369ea..799443326 100644 --- a/EasyPost.Tests/ServicesTests/ServiceTest.cs +++ b/EasyPost.Tests/ServicesTests/ServiceTest.cs @@ -28,7 +28,7 @@ public void TestServiceDisposal() Client client = new(new ClientConfiguration("not_a_real_api_key")); AddressService addressService = client.Address; - addressService.Dispose(); // Will also dispose the underlying client + addressService.Dispose(); // As long as this test doesn't throw an exception, it passes } diff --git a/EasyPost/_base/EasyPostService.cs b/EasyPost/_base/EasyPostService.cs index 61d9e6d09..7a3074e1c 100644 --- a/EasyPost/_base/EasyPostService.cs +++ b/EasyPost/_base/EasyPostService.cs @@ -73,8 +73,8 @@ protected virtual void Dispose(bool disposing) // Dispose managed state (managed objects) - // Attempt to dispose the client (may already be disposed) - Client.Dispose(); + // Don't dispose of the associated client here, as it may be shared among multiple services + // Client.Dispose(); } /// From f0ec5ea491dd56ba58332f0b6406381374ecffa5 Mon Sep 17 00:00:00 2001 From: Nate Harris Date: Tue, 17 Sep 2024 13:32:27 -0600 Subject: [PATCH 4/5] - Linting --- EasyPost.Tests/HttpTests/RequestTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/EasyPost.Tests/HttpTests/RequestTest.cs b/EasyPost.Tests/HttpTests/RequestTest.cs index edd01ccb6..e105717e3 100644 --- a/EasyPost.Tests/HttpTests/RequestTest.cs +++ b/EasyPost.Tests/HttpTests/RequestTest.cs @@ -12,7 +12,7 @@ public class RequestTest [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" } }); + Request request = new("https://google.com", "not_a_real_endpoint", Method.Get, ApiVersion.V2, new Dictionary { { "key", "value" } }, new Dictionary { { "header_key", "header_value" } }); request.Dispose(); From 85cb8a3e144ff5f9ae57e4aa06fb617237d73397 Mon Sep 17 00:00:00 2001 From: Nate Harris Date: Tue, 17 Sep 2024 14:22:13 -0600 Subject: [PATCH 5/5] - Address feedback - Add new assertion handler for asserting an action does not throw an exception --- EasyPost.Tests/ClientTest.cs | 6 +-- .../HttpTests/ClientConfigurationTest.cs | 6 +-- EasyPost.Tests/HttpTests/RequestTest.cs | 6 +-- EasyPost.Tests/ServicesTests/ServiceTest.cs | 5 +- .../_Utilities/Assertions/AnyException.cs | 2 +- .../Assertions/CollectionAsserts.cs | 6 --- .../Assertions/DoesNotThrowAssert.cs | 50 +++++++++++++++++++ .../Assertions/DoesNotThrowException.cs | 19 +++++++ .../Assertions/GuardArgumentNotNull.cs | 14 ++++++ EasyPost/_base/EasyPostService.cs | 1 - 10 files changed, 92 insertions(+), 23 deletions(-) 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/EasyPost.Tests/ClientTest.cs b/EasyPost.Tests/ClientTest.cs index 0e803a086..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 { @@ -74,9 +74,7 @@ public void TestThreadSafety() public void TestClientDisposal() { Client client = new(new ClientConfiguration(FakeApikey)); - client.Dispose(); - - // As long as this test doesn't throw an exception, it passes + CustomAssertions.DoesNotThrow(() => client.Dispose()); } [Fact] diff --git a/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs b/EasyPost.Tests/HttpTests/ClientConfigurationTest.cs index 69ee78201..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 { @@ -12,10 +13,7 @@ public class ClientConfigurationTest public void TestClientConfigurationDisposal() { ClientConfiguration configuration = new("not_a_real_api_key"); - - configuration.Dispose(); - - // As long as this test doesn't throw an exception, it passes + CustomAssertions.DoesNotThrow(() => configuration.Dispose()); } [Fact] diff --git a/EasyPost.Tests/HttpTests/RequestTest.cs b/EasyPost.Tests/HttpTests/RequestTest.cs index e105717e3..b97bcb546 100644 --- a/EasyPost.Tests/HttpTests/RequestTest.cs +++ b/EasyPost.Tests/HttpTests/RequestTest.cs @@ -2,6 +2,7 @@ using EasyPost._base; using EasyPost.Http; using Xunit; +using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert; namespace EasyPost.Tests.HttpTests; @@ -13,10 +14,7 @@ public class RequestTest 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" } }); - - request.Dispose(); - - // As long as this test doesn't throw an exception, it passes + CustomAssertions.DoesNotThrow(() => request.Dispose()); } #endregion diff --git a/EasyPost.Tests/ServicesTests/ServiceTest.cs b/EasyPost.Tests/ServicesTests/ServiceTest.cs index 799443326..7ece449ce 100644 --- a/EasyPost.Tests/ServicesTests/ServiceTest.cs +++ b/EasyPost.Tests/ServicesTests/ServiceTest.cs @@ -10,6 +10,7 @@ using EasyPost.Tests._Utilities.Attributes; using EasyPost.Utilities.Internal.Attributes; using Xunit; +using CustomAssertions = EasyPost.Tests._Utilities.Assertions.Assert; namespace EasyPost.Tests.ServicesTests { @@ -28,9 +29,7 @@ public void TestServiceDisposal() Client client = new(new ClientConfiguration("not_a_real_api_key")); AddressService addressService = client.Address; - addressService.Dispose(); - - // As long as this test doesn't throw an exception, it passes + CustomAssertions.DoesNotThrow(() => addressService.Dispose()); } /// 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/_base/EasyPostService.cs b/EasyPost/_base/EasyPostService.cs index 7a3074e1c..9f58c741e 100644 --- a/EasyPost/_base/EasyPostService.cs +++ b/EasyPost/_base/EasyPostService.cs @@ -74,7 +74,6 @@ protected virtual void Dispose(bool disposing) // Dispose managed state (managed objects) // Don't dispose of the associated client here, as it may be shared among multiple services - // Client.Dispose(); } ///