Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions EnumerableAsyncProcessor.Example/DisposalExample.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#if NET6_0_OR_GREATER
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -29,19 +28,19 @@ public static async Task RunExamples()
// Example 1: The problematic pattern from the issue
Console.WriteLine("Example 1: PROBLEMATIC - No disposal (resource leak!)");
var results1 = await ProblematicPatternAsync(new[] { 1, 2, 3, 4, 5 }, CancellationToken.None);
Console.WriteLine($"Results: {string.Join(", ", results1.ToList())}");
Console.WriteLine($"Results: {string.Join(", ", results1)}");
Console.WriteLine("⚠️ This pattern leaks resources because the processor is never disposed!\n");

// Example 2: Proper disposal with await using
Console.WriteLine("Example 2: PROPER - Using await using for automatic disposal");
var results2 = await ProperPatternWithAwaitUsingAsync(new[] { 1, 2, 3, 4, 5 }, CancellationToken.None);
Console.WriteLine($"Results: {string.Join(", ", results2.ToList())}");
Console.WriteLine($"Results: {string.Join(", ", results2)}");
Console.WriteLine("✅ Resources automatically cleaned up with await using\n");

// Example 3: Proper disposal with manual try-finally
Console.WriteLine("Example 3: PROPER - Manual disposal with try-finally");
var results3 = await ProperPatternWithManualDisposalAsync(new[] { 1, 2, 3, 4, 5 }, CancellationToken.None);
Console.WriteLine($"Results: {string.Join(", ", results3.ToList())}");
Console.WriteLine($"Results: {string.Join(", ", results3)}");
Console.WriteLine("✅ Resources manually cleaned up in finally block\n");

// Example 4: Using the convenience extension (no disposal needed)
Expand All @@ -65,23 +64,27 @@ public static async Task RunExamples()
/// This is the PROBLEMATIC pattern from the GitHub issue - it leaks resources!
/// DO NOT USE THIS PATTERN in production code.
/// </summary>
private static async Task<IAsyncEnumerable<int>> ProblematicPatternAsync(int[] input, CancellationToken token)
private static async Task<IReadOnlyList<int>> ProblematicPatternAsync(int[] input, CancellationToken token)
{
// ⚠️ PROBLEM: The processor is created but never disposed!
var batchProcessor = input.SelectAsync(static v => TransformAsync(v), token).ProcessInParallel();

// This returns the async enumerable, but the processor that created it is never disposed
return batchProcessor.GetResultsAsyncEnumerable();


var results = new List<int>();
await foreach (var result in batchProcessor.GetResultsAsyncEnumerable())
{
results.Add(result);
}

// 🔥 RESOURCE LEAK: The processor goes out of scope without being disposed,
// potentially leaving tasks running and resources uncleaned
return results;
Comment on lines 78 to +80

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Leak Description No Longer Matches

The new eager await foreach waits for every result before this method returns, so processor tasks are no longer left running when it goes out of scope. The processor still is not disposed, but this example now demonstrates leaked disposal resources rather than unfinished work.

Suggested change
// 🔥 RESOURCE LEAK: The processor goes out of scope without being disposed,
// potentially leaving tasks running and resources uncleaned
return results;
// 🔥 RESOURCE LEAK: The processor goes out of scope without being disposed,
// leaving its disposal resources uncleaned
return results;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 563971e. The comment now states that undisposed processor resources remain uncleaned; it no longer claims eager-enumerated tasks are still running.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: #347 auto-merged before the pushed review-fix commit was attached. The corrected leak description is now in follow-up #349 (e0059fc).

}

/// <summary>
/// PROPER pattern using await using for automatic disposal.
/// This is the recommended approach.
/// </summary>
private static async Task<IAsyncEnumerable<int>> ProperPatternWithAwaitUsingAsync(int[] input, CancellationToken token)
private static async Task<IReadOnlyList<int>> ProperPatternWithAwaitUsingAsync(int[] input, CancellationToken token)
{
// ✅ Create processor with await using for automatic disposal
await using var processor = input.SelectAsync(static v => TransformAsync(v), token).ProcessInParallel();
Expand All @@ -93,17 +96,15 @@ private static async Task<IAsyncEnumerable<int>> ProperPatternWithAwaitUsingAsyn
results.Add(result);
}

// Return as async enumerable
return results.ToAsyncEnumerable();

// ✅ Processor is automatically disposed here due to 'await using'
return results;
}

/// <summary>
/// PROPER pattern using manual disposal with try-finally.
/// Use this when you need more control over the disposal timing.
/// </summary>
private static async Task<IAsyncEnumerable<int>> ProperPatternWithManualDisposalAsync(int[] input, CancellationToken token)
private static async Task<IReadOnlyList<int>> ProperPatternWithManualDisposalAsync(int[] input, CancellationToken token)
{
var processor = input.SelectAsync(static v => TransformAsync(v), token).ProcessInParallel();

Expand All @@ -116,7 +117,7 @@ private static async Task<IAsyncEnumerable<int>> ProperPatternWithManualDisposal
results.Add(result);
}

return results.ToAsyncEnumerable();
return results;
}
finally
{
Expand Down Expand Up @@ -168,4 +169,4 @@ private static async IAsyncEnumerable<int> GenerateAsyncEnumerable(
}
}
}
#endif
#endif
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
using ModularPipelines.Context;
using ModularPipelines.DotNet.Extensions;
using ModularPipelines.DotNet.Options;
using ModularPipelines.Git.Extensions;
using ModularPipelines.Models;
using ModularPipelines.Modules;

namespace EnumerableAsyncProcessor.Pipeline.Modules;

public class BuildExampleProjectsModule : Module<List<CommandResult>>
{
protected override async Task<List<CommandResult>?> ExecuteAsync(
IModuleContext context,
CancellationToken cancellationToken)
{
var results = new List<CommandResult>();

foreach (var exampleProjectFile in context
.Git().RootDirectory
.GetFiles(file => file.Path.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)
&& file.Path.Contains("Example", StringComparison.OrdinalIgnoreCase)))
{
results.Add(await context.DotNet().Build(new DotNetBuildOptions
{
ProjectSolution = exampleProjectFile.Path,
Configuration = "Release",
}, cancellationToken: cancellationToken));
Comment on lines +22 to +27

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Failed Builds May Remain Successful

If DotNet().Build returns a nonzero CommandResult rather than throwing, this module stores that result and completes successfully. PackProjectsModule only depends on module completion and never checks these results, so packaging can continue after an example fails to compile.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 563971e. BuildExampleProjectsModule now passes CommandExecutionOptions { ThrowOnNonZeroExitCode = true }, so any failed example build throws and fails the module before packaging can run.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: #347 auto-merged before the pushed review-fix commit was attached. The same fix is now isolated on current main in #349 (e0059fc), with the example and pipeline builds passing.

}

return results;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace EnumerableAsyncProcessor.Pipeline.Modules;
[DependsOn<PackageFilesRemovalModule>]
[DependsOn<NugetVersionGeneratorModule>]
[DependsOn<RunUnitTestsModule>]
[DependsOn<BuildExampleProjectsModule>]
public class PackProjectsModule : Module<List<CommandResult>>
{
protected override async Task<List<CommandResult>?> ExecuteAsync(IModuleContext context, CancellationToken cancellationToken)
Expand Down
3 changes: 2 additions & 1 deletion EnumerableAsyncProcessor.Pipeline/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
builder.AddModule<UploadPackagesToNugetModule>();
}

builder.AddModule<RunUnitTestsModule>()
builder.AddModule<BuildExampleProjectsModule>()
.AddModule<RunUnitTestsModule>()
.AddModule<NugetVersionGeneratorModule>()
.AddModule<PackProjectsModule>()
.AddModule<PackageFilesRemovalModule>()
Expand Down
Loading