diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f758ed13..5f999463 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,9 +99,9 @@ jobs: - name: Upload coverage to Codecov uses: codecov/codecov-action@v6 with: - files: | - **/TestResults/*/coverage.cobertura.xml + files: ./coverage-report/Cobertura.xml flags: unittests + disable_search: true fail_ci_if_error: true verbose: true env: diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 30c2250d..9c2fdc55 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -95,17 +95,6 @@ jobs: TestResults/**/*.trx check_name: Test Results - - name: Upload coverage to Codecov - uses: codecov/codecov-action@v6 - if: always() - with: - files: ./TestResults/**/coverage.opencover.xml - flags: unittests - name: pr-${{ github.event.pull_request.number }} - fail_ci_if_error: false - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - - name: Build Documentation run: | dotnet tool update -g docfx diff --git a/docs/generators/composer.md b/docs/generators/composer.md index bc7ee54d..ff8579cd 100644 --- a/docs/generators/composer.md +++ b/docs/generators/composer.md @@ -90,7 +90,7 @@ ValueTask StepNameAsync(TIn input, Func> next, Cancel - `input`: The pipeline input (not `in` for async) - `next`: Async delegate to call the next step -- `ct`: CancellationToken for cooperative cancellation +- `ct`: Optional CancellationToken for cooperative cancellation. Async steps may omit it when they do not need cancellation. ## Terminal Method Signature @@ -106,6 +106,8 @@ TOut TerminalName(in TIn input) ValueTask TerminalNameAsync(TIn input, CancellationToken ct) ``` +Async terminals may omit the `CancellationToken` parameter when they do not need cancellation. + ## Attributes ### `[Composer]` @@ -116,7 +118,7 @@ Main attribute for marking pipeline host types. |---|---|---|---| | `InvokeMethodName` | `string` | `"Invoke"` | Name of generated sync method | | `InvokeAsyncMethodName` | `string` | `"InvokeAsync"` | Name of generated async method | -| `GenerateAsync` | `bool` | Inferred when omitted | Explicit async control | +| `GenerateAsync` | `bool` | Inferred by generator when the named property is omitted | Explicit async control | | `ForceAsync` | `bool` | `false` | Force async generation even if all steps are sync | | `WrapOrder` | `ComposerWrapOrder` | `OuterFirst` | Determines wrapping order | @@ -206,7 +208,7 @@ public ValueTask InvokeAsync(string input, CancellationToken ct = defaul | **PKCOM006** | Error | Invalid step method signature | | **PKCOM007** | Error | Invalid terminal method signature | | **PKCOM008** | Error | Async step detected but async generation disabled | -| **PKCOM009** | Warning | Async method missing CancellationToken parameter | +| **PKCOM009** | Warning | Async method declares a final extra parameter with the wrong type | ## Best Practices diff --git a/src/PatternKit.Core/Creational/ObjectPool/ObjectPool.cs b/src/PatternKit.Core/Creational/ObjectPool/ObjectPool.cs index 77f8bfc4..2fec05b9 100644 --- a/src/PatternKit.Core/Creational/ObjectPool/ObjectPool.cs +++ b/src/PatternKit.Core/Creational/ObjectPool/ObjectPool.cs @@ -8,6 +8,7 @@ namespace PatternKit.Creational.ObjectPool; public sealed class ObjectPool : IDisposable { private readonly ConcurrentQueue _items = new(); + private readonly object _gate = new(); private readonly Func _factory; private readonly Action? _onRent; private readonly Action? _onReturn; @@ -34,17 +35,38 @@ private ObjectPool(Func factory, Action? onRent, Action? onReturn, Func /// Rents an item from the pool. Dispose the returned lease to return the item. public ObjectPoolLease Rent() { - ThrowIfDisposed(); - T value; - if (_items.TryDequeue(out var pooled)) + var hasValue = false; + lock (_gate) { - Interlocked.Decrement(ref _retained); - value = pooled; + ThrowIfDisposed(); + + if (_items.TryDequeue(out var pooled)) + { + Interlocked.Decrement(ref _retained); + value = pooled; + hasValue = true; + } + else + { + value = default!; + } } - else - { + + if (!hasValue) value = _factory(); + + var disposeAfterRent = false; + lock (_gate) + { + if (_disposed) + disposeAfterRent = true; + } + + if (disposeAfterRent) + { + DisposeIfNeeded(value); + throw new ObjectDisposedException(nameof(ObjectPool)); } _onRent?.Invoke(value); @@ -53,44 +75,82 @@ public ObjectPoolLease Rent() internal void Return(T value) { - if (_disposed) + var disposeImmediately = false; + lock (_gate) + { + disposeImmediately = _disposed; + } + + if (disposeImmediately) { DisposeIfNeeded(value); return; } - _onReturn?.Invoke(value); - if (_shouldReturn is not null && !_shouldReturn(value)) + try + { + _onReturn?.Invoke(value); + if (_shouldReturn is not null && !_shouldReturn(value)) + { + DisposeIfNeeded(value); + return; + } + } + catch { DisposeIfNeeded(value); - return; + throw; } - var retained = Interlocked.Increment(ref _retained); - if (retained <= _maxRetained) + var dispose = false; + lock (_gate) { - _items.Enqueue(value); - return; + if (_disposed) + { + dispose = true; + } + else + { + var retained = Interlocked.Increment(ref _retained); + if (retained <= _maxRetained) + { + _items.Enqueue(value); + return; + } + + Interlocked.Decrement(ref _retained); + dispose = true; + } } - Interlocked.Decrement(ref _retained); - DisposeIfNeeded(value); + if (dispose) + DisposeIfNeeded(value); } /// public void Dispose() { - _disposed = true; - while (_items.TryDequeue(out var value)) + var drained = new List(); + lock (_gate) { - Interlocked.Decrement(ref _retained); - DisposeIfNeeded(value); + if (_disposed) + return; + + _disposed = true; + while (_items.TryDequeue(out var value)) + { + Interlocked.Decrement(ref _retained); + drained.Add(value); + } } + + foreach (var value in drained) + DisposeIfNeeded(value); } private void ThrowIfDisposed() { - if (_disposed) + if (Volatile.Read(ref _disposed)) throw new ObjectDisposedException(nameof(ObjectPool)); } diff --git a/src/PatternKit.Generators.Abstractions/Composer/ComposerAttribute.cs b/src/PatternKit.Generators.Abstractions/Composer/ComposerAttribute.cs index d1a00cd2..0680c794 100644 --- a/src/PatternKit.Generators.Abstractions/Composer/ComposerAttribute.cs +++ b/src/PatternKit.Generators.Abstractions/Composer/ComposerAttribute.cs @@ -21,8 +21,8 @@ public sealed class ComposerAttribute : Attribute /// /// Gets or sets whether to generate async methods. - /// When omitted, async generation is inferred from the presence of async steps or terminal. - /// Set to true/false explicitly to control async generation. + /// When this named property is omitted, async generation is inferred from async steps or terminal. + /// Set to true or false explicitly in source to control async generation. /// public bool GenerateAsync { get; set; } diff --git a/src/PatternKit.Generators.Abstractions/Proxy/GenerateProxyAttribute.cs b/src/PatternKit.Generators.Abstractions/Proxy/GenerateProxyAttribute.cs index d52a2d21..b23e8451 100644 --- a/src/PatternKit.Generators.Abstractions/Proxy/GenerateProxyAttribute.cs +++ b/src/PatternKit.Generators.Abstractions/Proxy/GenerateProxyAttribute.cs @@ -73,7 +73,7 @@ public sealed class GenerateProxyAttribute : Attribute /// /// Gets or sets whether async interceptor hooks should be generated. - /// If not specified, async support is inferred from the contract + /// When this named property is omitted, async hook generation is inferred from the contract /// (enabled if any member returns Task/ValueTask or has a CancellationToken parameter). /// public bool GenerateAsync { get; set; } diff --git a/src/PatternKit.Generators/ComposerGenerator.cs b/src/PatternKit.Generators/ComposerGenerator.cs index 374753e4..61fc9b6e 100644 --- a/src/PatternKit.Generators/ComposerGenerator.cs +++ b/src/PatternKit.Generators/ComposerGenerator.cs @@ -90,8 +90,8 @@ public sealed class ComposerGenerator : IIncrementalGenerator private static readonly DiagnosticDescriptor MissingCancellationTokenDescriptor = new( id: DiagIdMissingCancellationToken, - title: "CancellationToken parameter required", - messageFormat: "Method '{0}' is async but missing CancellationToken parameter. Async methods should have a CancellationToken parameter.", + title: "CancellationToken parameter is invalid", + messageFormat: "Method '{0}' is async and declares a final extra parameter, but it is not System.Threading.CancellationToken", category: "PatternKit.Generators.Composer", defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true); diff --git a/test/PatternKit.Examples.Tests/Chain/MediatedTransactionPipelineDemoTests.cs b/test/PatternKit.Examples.Tests/Chain/MediatedTransactionPipelineDemoTests.cs index 6a0a4b4e..181c1079 100644 --- a/test/PatternKit.Examples.Tests/Chain/MediatedTransactionPipelineDemoTests.cs +++ b/test/PatternKit.Examples.Tests/Chain/MediatedTransactionPipelineDemoTests.cs @@ -284,7 +284,7 @@ public void ComputeDelta_Rounds_To_Nearest_Nickel() public sealed class MediatedTransactionPipelineCoverageTests { - [Scenario("TransactionPipelineBuilder FluentHooks Cover CustomStagesRulesAndCouponDiscounts")] + [Scenario("Transaction pipeline builder fluent hooks cover custom stages rules and coupon discounts")] [Fact] public void TransactionPipelineBuilder_FluentHooks_Cover_CustomStagesRulesAndCouponDiscounts() { @@ -327,7 +327,7 @@ public void TransactionPipelineBuilder_FluentHooks_Cover_CustomStagesRulesAndCou ScenarioExpect.Equal("custom", customResult.Result.Code); } - [Scenario("TransactionPipelineBuilder Preauth Blocks EmptyBaskets")] + [Scenario("Transaction pipeline builder preauth blocks empty baskets")] [Fact] public void TransactionPipelineBuilder_Preauth_Blocks_EmptyBaskets() { @@ -347,7 +347,7 @@ public void TransactionPipelineBuilder_Preauth_Blocks_EmptyBaskets() ScenarioExpect.True(result.Ctx.Log.Contains("preauth: empty basket")); } - [Scenario("CardTenderHandlers Surface AuthorizationAndCaptureFailures")] + [Scenario("Card tender handlers surface authorization and capture failures")] [Fact] public void CardTenderHandlers_Surface_AuthorizationAndCaptureFailures() { @@ -396,7 +396,7 @@ public void CardTenderHandlers_Surface_AuthorizationAndCaptureFailures() ScenarioExpect.Equal("capture-failed", strategyCaptureResult!.Value.Code); } - [Scenario("CharityRoundUpRule NotifiesTrackerWhenApplied")] + [Scenario("Charity round up rule notifies tracker when applied")] [Fact] public void CharityRoundUpRule_NotifiesTracker_WhenApplied() { diff --git a/test/PatternKit.Examples.Tests/Generators/FacadeSpecsTests.cs b/test/PatternKit.Examples.Tests/Generators/FacadeSpecsTests.cs index e0dc96e9..54142290 100644 --- a/test/PatternKit.Examples.Tests/Generators/FacadeSpecsTests.cs +++ b/test/PatternKit.Examples.Tests/Generators/FacadeSpecsTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using PatternKit.Examples.Generators.Facade; using TinyBDD; @@ -9,7 +10,7 @@ namespace PatternKit.Examples.Tests.Generators; /// public class FacadeSpecsTests { - [Scenario("ShippingFacade CalculatesCostCorrectly")] + [Scenario("Shipping facade calculates cost correctly")] [Fact] public void ShippingFacade_CalculatesCostCorrectly() { @@ -28,7 +29,7 @@ public void ShippingFacade_CalculatesCostCorrectly() ScenarioExpect.Equal(5.99m, cost); // local base rate, 3.5 lbs (under 5 lbs, no surcharge) } - [Scenario("ShippingFacade ValidatesShipmentCorrectly")] + [Scenario("Shipping facade validates shipment correctly")] [Fact] public void ShippingFacade_ValidatesShipmentCorrectly() { @@ -53,7 +54,7 @@ public void ShippingFacade_ValidatesShipmentCorrectly() ScenarioExpect.False(isInvalid); } - [Scenario("ShippingFacade EstimatesDeliveryCorrectly")] + [Scenario("Shipping facade estimates delivery correctly")] [Fact] public void ShippingFacade_EstimatesDeliveryCorrectly() { @@ -74,7 +75,7 @@ public void ShippingFacade_EstimatesDeliveryCorrectly() ScenarioExpect.True(days > 0 && days <= 12); } - [Scenario("ShippingSubsystems CoverFallbackDestinationsAndSpeeds")] + [Scenario("Shipping subsystems cover fallback destinations and speeds")] [Fact] public void ShippingSubsystems_CoverFallbackDestinationsAndSpeeds() { @@ -88,7 +89,7 @@ public void ShippingSubsystems_CoverFallbackDestinationsAndSpeeds() ScenarioExpect.Equal(29.99m, rates.CalculateBaseRate("international")); ScenarioExpect.Equal(1.50m, rates.CalculateWeightSurcharge(8m)); ScenarioExpect.Equal(12, estimator.EstimateDays("international", "economy")); - ScenarioExpect.Equal("$31.49 - Delivery in 12 business days", quote); + ScenarioExpect.Equal($"${31.49m.ToString("F2", CultureInfo.CurrentCulture)} - Delivery in 12 business days", quote); ScenarioExpect.Equal("Invalid shipment parameters", invalidQuote); } diff --git a/test/PatternKit.Tests/Creational/ObjectPool/ObjectPoolTests.cs b/test/PatternKit.Tests/Creational/ObjectPool/ObjectPoolTests.cs index ed2d6bd0..190cbbae 100644 --- a/test/PatternKit.Tests/Creational/ObjectPool/ObjectPoolTests.cs +++ b/test/PatternKit.Tests/Creational/ObjectPool/ObjectPoolTests.cs @@ -52,6 +52,27 @@ public Task Retention_Predicate_Discards_Unhealthy_Items() .Then("the item is not retained", retained => ScenarioExpect.Equal(0, retained)) .AssertPassed(); + [Scenario("Retention predicate disposes discarded disposable items")] + [Fact] + public Task Retention_Predicate_Disposes_Discarded_Disposable_Items() + => Given("an object pool with disposable validation", () => ObjectPool.Create() + .WithFactory(static () => new DisposableBuffer()) + .RetainWhen(static _ => false) + .Build()) + .When("returning an item rejected by the predicate", pool => + { + var lease = pool.Rent(); + var item = lease.Value; + lease.Dispose(); + return new { pool.RetainedCount, item.IsDisposed }; + }) + .Then("the item is disposed instead of retained", result => + { + ScenarioExpect.Equal(0, result.RetainedCount); + ScenarioExpect.True(result.IsDisposed); + }) + .AssertPassed(); + [Scenario("Max retained prevents unbounded growth")] [Fact] public Task Max_Retained_Prevents_Unbounded_Growth() @@ -72,6 +93,29 @@ public Task Max_Retained_Prevents_Unbounded_Growth() .Then("only the configured number of items is retained", retained => ScenarioExpect.Equal(2, retained)) .AssertPassed(); + [Scenario("Max retained disposes disposable items above capacity")] + [Fact] + public Task Max_Retained_Disposes_Disposable_Items_Above_Capacity() + => Given("an object pool with a one item retention limit", () => ObjectPool.Create() + .WithFactory(static () => new DisposableBuffer()) + .WithMaxRetained(1) + .Build()) + .When("returning more disposable items than the pool can retain", pool => + { + var first = pool.Rent(); + var second = pool.Rent(); + var overflow = second.Value; + first.Dispose(); + second.Dispose(); + return new { pool.RetainedCount, overflow.IsDisposed }; + }) + .Then("only one item is retained and the overflow is disposed", result => + { + ScenarioExpect.Equal(1, result.RetainedCount); + ScenarioExpect.True(result.IsDisposed); + }) + .AssertPassed(); + [Scenario("Disposed pool rejects future rents")] [Fact] public Task Disposed_Pool_Rejects_Future_Rents() @@ -84,6 +128,135 @@ public Task Disposed_Pool_Rejects_Future_Rents() .Then("renting fails", pool => ScenarioExpect.Throws(() => pool.Rent())) .AssertPassed(); + [Scenario("Disposing a pool releases retained disposable items")] + [Fact] + public Task Disposing_A_Pool_Releases_Retained_Disposable_Items() + => Given("an object pool with a returned disposable item", () => + { + var pool = ObjectPool.Create() + .WithFactory(static () => new DisposableBuffer()) + .WithMaxRetained(1) + .Build(); + var lease = pool.Rent(); + var item = lease.Value; + lease.Dispose(); + return new { Pool = pool, Item = item }; + }) + .When("the pool is disposed", ctx => + { + ctx.Pool.Dispose(); + return new { ctx.Pool.RetainedCount, ctx.Item.IsDisposed }; + }) + .Then("the retained item is disposed and removed", result => + { + ScenarioExpect.Equal(0, result.RetainedCount); + ScenarioExpect.True(result.IsDisposed); + }) + .AssertPassed(); + + [Scenario("Returning a leased item after dispose releases the item")] + [Fact] + public Task Returning_A_Leased_Item_After_Dispose_Releases_The_Item() + => Given("an object pool with a rented disposable item", () => + { + var pool = ObjectPool.Create() + .WithFactory(static () => new DisposableBuffer()) + .WithMaxRetained(1) + .Build(); + var lease = pool.Rent(); + return new { Pool = pool, Lease = lease, Item = lease.Value }; + }) + .When("the pool is disposed before the lease is returned", ctx => + { + ctx.Pool.Dispose(); + ctx.Lease.Dispose(); + return new { ctx.Pool.RetainedCount, ctx.Item.IsDisposed }; + }) + .Then("the returned item is disposed instead of retained", result => + { + ScenarioExpect.Equal(0, result.RetainedCount); + ScenarioExpect.True(result.IsDisposed); + }) + .AssertPassed(); + + [Scenario("Returning a leased item after dispose skips throwing callbacks")] + [Fact] + public Task Returning_A_Leased_Item_After_Dispose_Skips_Throwing_Callbacks() + => Given("an object pool with a throwing return callback and a rented disposable item", () => + { + var pool = ObjectPool.Create() + .WithFactory(static () => new DisposableBuffer()) + .OnReturn(static _ => throw new InvalidOperationException("return callback should not run")) + .RetainWhen(static _ => throw new InvalidOperationException("retention callback should not run")) + .WithMaxRetained(1) + .Build(); + var lease = pool.Rent(); + return new { Pool = pool, Lease = lease, Item = lease.Value }; + }) + .When("the pool is disposed before the lease is returned", ctx => + { + ctx.Pool.Dispose(); + ctx.Lease.Dispose(); + return new { ctx.Pool.RetainedCount, ctx.Item.IsDisposed }; + }) + .Then("the callbacks are skipped and the returned item is disposed", result => + { + ScenarioExpect.Equal(0, result.RetainedCount); + ScenarioExpect.True(result.IsDisposed); + }) + .AssertPassed(); + + [Scenario("Throwing return callbacks dispose the item before rethrowing")] + [Fact] + public Task Throwing_Return_Callbacks_Dispose_The_Item_Before_Rethrowing() + => Given("an object pool with a throwing return callback and a rented disposable item", () => + { + var pool = ObjectPool.Create() + .WithFactory(static () => new DisposableBuffer()) + .OnReturn(static _ => throw new InvalidOperationException("return callback failed")) + .WithMaxRetained(1) + .Build(); + var lease = pool.Rent(); + return new { Lease = lease, Item = lease.Value }; + }) + .When("returning the lease", ctx => + { + ScenarioExpect.Throws(() => ctx.Lease.Dispose()); + return ctx.Item.IsDisposed; + }) + .Then("the item is disposed before the callback failure is rethrown", disposed => ScenarioExpect.True(disposed)) + .AssertPassed(); + + [Scenario("Disposing during item creation releases the created item")] + [Fact] + public Task Disposing_During_Item_Creation_Releases_The_Created_Item() + => Given("an object pool that is disposed by its factory", () => + { + ObjectPool? pool = null; + DisposableBuffer? item = null; + pool = ObjectPool.Create() + .WithFactory(() => + { + item = new DisposableBuffer(); + pool!.Dispose(); + return item; + }) + .Build(); + + return new { Pool = pool, Item = new Func(() => item) }; + }) + .When("renting causes the factory to dispose the pool", ctx => + { + ScenarioExpect.Throws(() => ctx.Pool.Rent()); + return ctx.Item(); + }) + .Then("the created item is disposed instead of leaked", item => + { + ScenarioExpect.NotNull(item); + ScenarioExpect.True(item!.IsDisposed); + }) + .AssertPassed(); + [Scenario("Invalid builder configuration is rejected")] [Fact] public Task Invalid_Builder_Configuration_Is_Rejected() @@ -108,4 +281,11 @@ private sealed class PooledBuffer public void Reset() => _segments.Clear(); } + + private sealed class DisposableBuffer : IDisposable + { + public bool IsDisposed { get; private set; } + + public void Dispose() => IsDisposed = true; + } }