Conversation
There was a problem hiding this comment.
Pull request overview
This pull request comprehensively refactors the async archive and writer APIs to improve performance, add proper cancellation support, and return ValueTask for better async efficiency. The changes affect factory interfaces, implementations, utility methods, and test files across the codebase.
Changes:
- Converted factory async methods (
OpenAsyncArchive,OpenAsyncWriter) to returnValueTask<T>and acceptCancellationTokenparameters for improved async performance and proper cancellation support - Added new async file stream utility methods (
OpenAsyncReadStream,OpenAsyncWriteStream) that useFile.OpenHandlewithFileOptions.Asynchronouson .NET 8.0+ for optimal performance - Extended
WriterOptionsExtensionswith polymorphicWithLeaveStreamOpenmethod supporting allIWriterOptionsimplementations - Updated all test files to properly await the new async factory methods
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/SharpCompress/Archives/IMultiArchiveFactory.cs |
Updated interface methods to return ValueTask<IAsyncArchive> and accept CancellationToken |
src/SharpCompress/Archives/ArchiveFactory.Async.cs |
Updated internal factory calls to properly await and pass cancellation tokens through |
src/SharpCompress/Factories/GZipFactory.cs |
Implemented async factory methods returning ValueTask with cancellation support |
src/SharpCompress/Factories/RarFactory.cs |
Implemented async factory methods returning ValueTask with cancellation support |
src/SharpCompress/Factories/SevenZipFactory.cs |
Implemented async factory methods returning ValueTask with cancellation support |
src/SharpCompress/Factories/TarFactory.cs |
Implemented async factory methods returning ValueTask with cancellation support |
src/SharpCompress/Factories/ZipFactory.cs |
Implemented async factory methods returning ValueTask with cancellation support |
src/SharpCompress/Writers/IWriterFactory.cs |
Updated interface to return ValueTask<IAsyncWriter> |
src/SharpCompress/Writers/WriterFactory.cs |
Updated factory methods to return ValueTask<IAsyncWriter> and use async file streams; added WithLeaveStreamOpen calls |
src/SharpCompress/Writers/WriterOptionsExtensions.cs |
Added polymorphic WithLeaveStreamOpen extension method for IWriterOptions interface |
src/SharpCompress/Utility.Async.cs |
Added OpenAsyncReadStream and OpenAsyncWriteStream helper methods with platform-specific optimizations |
src/SharpCompress/Readers/ReaderFactory.Async.cs |
Updated to use async file stream utilities for opening files |
src/SharpCompress/Readers/Tar/TarReader.Factory.cs |
Updated to use async file stream utilities for opening files |
tests/SharpCompress.Test/ArchiveTests.cs |
Updated CreateWriterWithLevelAsync to return ValueTask and properly await factory calls |
tests/SharpCompress.Test/WriterTests.cs |
Added await to OpenAsyncWriter call |
tests/SharpCompress.Test/Tar/TarArchiveAsyncTests.cs |
Added await to OpenAsyncWriter calls |
tests/SharpCompress.Test/GZip/GZipWriterAsyncTests.cs |
Added await to OpenAsyncWriter call |
tests/SharpCompress.Test/GZip/AsyncTests.cs |
Added await to OpenAsyncWriter call |
tests/SharpCompress.Test/Zip/ZipMemoryArchiveWithCrcAsyncTests.cs |
Added await to OpenAsyncWriter and CreateWriterWithLevelAsync calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # src/SharpCompress/Writers/WriterOptionsExtensions.cs # tests/SharpCompress.Test/GZip/GZipWriterAsyncTests.cs # tests/SharpCompress.Test/WriterTests.cs
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return OpenWriter( | ||
| fileInfo.OpenWrite(), | ||
| archiveType, | ||
| writerOptions.WithLeaveStreamOpen(false) |
There was a problem hiding this comment.
WARNING: WithLeaveStreamOpen(false) throws for custom IWriterOptions implementations
IWriterOptions is an interface and consumers can pass their own implementations. The new WithLeaveStreamOpen(bool) extension only supports a fixed set of record types and throws NotSupportedException for anything else, which is a behavioral break compared to the previous pass-through. Consider keeping the original options instance when the type is unknown (or provide a safe fallback) to preserve compatibility.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (5)
tests/SharpCompress.Test/GZip/GZipWriterAsyncTests.cs:32
IAsyncWriterimplementsIAsyncDisposable; usingusinghere will callDispose()instead ofDisposeAsync(). In async tests this can skip async flush/finalization and cause intermittent failures (especially on legacy frameworks). Preferawait usingfor writers opened viaOpenAsyncWriter.
using (
var writer = await WriterFactory.OpenAsyncWriter(
new AsyncOnlyStream(stream),
ArchiveType.GZip,
new WriterOptions(CompressionType.GZip)
)
)
tests/SharpCompress.Test/Zip/ZipMemoryArchiveWithCrcAsyncTests.cs:143
WriterFactory.OpenAsyncWriter(...)returnsIAsyncWriter(IAsyncDisposable). Disposing it synchronously viausingcan skip async finalization; preferawait usingfor this block.
using (
var writer = await WriterFactory.OpenAsyncWriter(
new AsyncOnlyStream(zipStream),
ArchiveType.Zip,
writerOptions
)
)
tests/SharpCompress.Test/Tar/TarArchiveAsyncTests.cs:43
IAsyncWriterisIAsyncDisposable; usingusingwill dispose synchronously. Useawait usinghere to ensure the async writer finalizes and flushes correctly before the tar file is read back.
using (
var writer = await WriterFactory.OpenAsyncWriter(
new AsyncOnlyStream(stream),
ArchiveType.Tar,
new WriterOptions(CompressionType.None) { LeaveStreamOpen = false }
)
)
tests/SharpCompress.Test/Tar/TarArchiveAsyncTests.cs:102
IAsyncWriterimplementsIAsyncDisposable; this should beawait usingsoDisposeAsync()runs (important for flushing/completing archive output) before subsequent verification.
using (
var writer = await WriterFactory.OpenAsyncWriter(
new AsyncOnlyStream(stream),
ArchiveType.Tar,
new WriterOptions(CompressionType.None) { LeaveStreamOpen = false }
)
)
tests/SharpCompress.Test/GZip/AsyncTests.cs:112
OpenAsyncWriterreturns anIAsyncWriter(IAsyncDisposable). Usingusinghere callsDispose()instead ofDisposeAsync(), which can leave the ZIP not fully finalized before you reopen it. Preferawait usingfor the writer block.
using (
var writer = await WriterFactory.OpenAsyncWriter(
new AsyncOnlyStream(stream),
ArchiveType.Zip,
new WriterOptions(CompressionType.Deflate) { LeaveStreamOpen = false }
)
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using ( | ||
| var writer = CreateWriterWithLevelAsync(zipStream, compressionType, compressionLevel) | ||
| var writer = await CreateWriterWithLevelAsync( | ||
| zipStream, | ||
| compressionType, | ||
| compressionLevel | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This async writer is IAsyncDisposable; using will invoke synchronous disposal. Use await using so DisposeAsync() runs and the in-memory zip is fully finalized before verification.
| fileInfo.NotNull(nameof(fileInfo)); | ||
| return OpenWriter(fileInfo.OpenWrite(), archiveType, writerOptions); | ||
| return OpenWriter( | ||
| fileInfo.OpenWrite(), | ||
| archiveType, | ||
| writerOptions.WithLeaveStreamOpen(false) | ||
| ); |
There was a problem hiding this comment.
fileInfo.OpenWrite() creates a stream that won't be disposed if OpenWriter(...) throws (e.g., unsupported archive type/options). Consider disposing the file stream on failure (e.g., open the stream into a local, try to create the writer, and Dispose() the stream in a catch/finally when writer creation fails).
| return OpenWriter( | ||
| fileInfo.OpenWrite(), | ||
| archiveType, | ||
| writerOptions.WithLeaveStreamOpen(false) | ||
| ); |
There was a problem hiding this comment.
writerOptions.WithLeaveStreamOpen(false) can throw NotSupportedException for any custom IWriterOptions implementation not covered by the switch, which is a breaking behavior change for public OpenWriter/OpenAsyncWriter(FileInfo, ...) overloads. Consider avoiding throwing when LeaveStreamOpen is already the requested value, and/or using a different mechanism to ensure owned file streams are disposed without requiring options cloning.
| return await OpenAsyncWriter( | ||
| stream, | ||
| archiveType, | ||
| writerOptions.WithLeaveStreamOpen(false), | ||
| cancellationToken | ||
| ) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
The file stream opened here (OpenAsyncWriteStream) will leak if OpenAsyncWriter(stream, ...) throws before returning the writer. Wrap writer creation in a try/catch/finally (or equivalent) so the stream is disposed on failure.
| return await OpenAsyncWriter( | |
| stream, | |
| archiveType, | |
| writerOptions.WithLeaveStreamOpen(false), | |
| cancellationToken | |
| ) | |
| .ConfigureAwait(false); | |
| try | |
| { | |
| return await OpenAsyncWriter( | |
| stream, | |
| archiveType, | |
| writerOptions.WithLeaveStreamOpen(false), | |
| cancellationToken | |
| ) | |
| .ConfigureAwait(false); | |
| } | |
| catch | |
| { | |
| stream.Dispose(); | |
| throw; | |
| } |
| return await OpenAsyncReader(stream, readerOptions, cancellationToken) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
The file stream opened here isn’t disposed if OpenAsyncReader(stream, ...) throws before returning a reader. Consider disposing stream on failure (try/finally) to prevent leaking file handles.
| return await OpenAsyncReader(stream, readerOptions, cancellationToken) | |
| .ConfigureAwait(false); | |
| try | |
| { | |
| return await OpenAsyncReader(stream, readerOptions, cancellationToken) | |
| .ConfigureAwait(false); | |
| } | |
| catch | |
| { | |
| stream.Dispose(); | |
| throw; | |
| } |
| using ( | ||
| var writer = CreateWriterWithLevelAsync(zipStream, compressionType, compressionLevel) | ||
| var writer = await CreateWriterWithLevelAsync( | ||
| zipStream, | ||
| compressionType, | ||
| compressionLevel | ||
| ) | ||
| ) |
There was a problem hiding this comment.
IAsyncWriter is IAsyncDisposable; this using will not await disposal. Use await using so any async flush/finalization completes before reopening the archive.
| public static IWriterOptions WithLeaveStreamOpen( | ||
| this IWriterOptions options, | ||
| bool leaveStreamOpen | ||
| ) => | ||
| options switch | ||
| { | ||
| WriterOptions writerOptions => writerOptions with { LeaveStreamOpen = leaveStreamOpen }, | ||
| ZipWriterOptions zipOptions => zipOptions with { LeaveStreamOpen = leaveStreamOpen }, | ||
| TarWriterOptions tarOptions => tarOptions with { LeaveStreamOpen = leaveStreamOpen }, | ||
| GZipWriterOptions gzipOptions => gzipOptions with { LeaveStreamOpen = leaveStreamOpen }, | ||
| _ => throw new NotSupportedException( | ||
| $"Cannot set LeaveStreamOpen on options of type {options.GetType().Name}. " | ||
| + "Options must be a record type implementing IWriterOptions." | ||
| ), | ||
| }; |
There was a problem hiding this comment.
This helper throws for any IWriterOptions implementation outside the known record types, even when no change is needed. At minimum, consider returning options directly when options.LeaveStreamOpen == leaveStreamOpen to avoid surprising NotSupportedExceptions for custom options that already have the desired value.
| options ??= ReaderOptions.ForOwnedFile; | ||
| return OpenAsyncReader(fileInfo.OpenRead(), options, cancellationToken); | ||
| var stream = fileInfo.OpenAsyncReadStream(cancellationToken); | ||
| return await OpenAsyncReader(stream, options, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
The stream opened from fileInfo.OpenAsyncReadStream(...) is not disposed if OpenAsyncReader(stream, ...) throws (e.g., invalid format). Wrap the call so the stream is disposed on failure to avoid file handle leaks.
| return await OpenAsyncReader(stream, options, cancellationToken).ConfigureAwait(false); | |
| try | |
| { | |
| return await OpenAsyncReader(stream, options, cancellationToken).ConfigureAwait(false); | |
| } | |
| catch | |
| { | |
| stream.Dispose(); | |
| throw; | |
| } |
| <DefineConstants>$(DefineConstants);LEGACY_DOTNET</DefineConstants> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net8.0' Or '$(TargetFramework)' == 'net9.0' Or '$(TargetFramework)' == 'net10.0' "> | ||
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net8.0' Or '$(TargetFramework)' == 'net10.0' "> |
There was a problem hiding this comment.
net9.0 is still listed in TargetFrameworks, but it was removed from the IsTrimmable/IsAotCompatible condition. If net9.0 is intended to remain supported, consider adding it back to this property group (or document why it’s intentionally excluded).
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net8.0' Or '$(TargetFramework)' == 'net10.0' "> | |
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net8.0' Or '$(TargetFramework)' == 'net9.0' Or '$(TargetFramework)' == 'net10.0' "> |
This pull request refactors and improves the asynchronous archive and writer APIs throughout the codebase. The main goals are to add proper support for cancellation tokens, return
ValueTaskfor better async performance, and provide optimized file stream helpers for asynchronous reading and writing, especially on .NET 8.0+. These changes make the library's async APIs safer, more consistent, and better performing.API Consistency and Async Improvements:
IMultiArchiveFactoryinterface and all implementing classes (e.g.,GZipFactory,RarFactory,SevenZipFactory,TarFactory,ZipFactory) so thatOpenAsyncArchivenow returns aValueTask<IAsyncArchive>and accepts aCancellationToken. This ensures proper async/cancellation support across all archive types. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]OpenAsyncWritermethods in writer factories to returnValueTask<IAsyncWriter>and properly support cancellation tokens. [1] [2] [3]Async File Stream Utilities:
Utility.Async.csfor opening file streams asynchronously for both reading and writing. These helpers useFile.OpenHandlewithFileOptions.Asynchronouson .NET 8.0+ for optimal performance and fall back to the legacyFileStreamconstructor on older frameworks.Reader Factory Async Improvements:
OpenAsyncReadermethods inReaderFactory.Async.csandTarReader.Factory.csto use the new async file stream utilities, ensuring that file streams are opened asynchronously and cancellation tokens are respected. [1] [2]Internal Refactoring for Cancellation Support:
ArchiveFactory.Async.csto ensure that cancellation tokens are properly passed through and awaited, further improving cancellation support in async archive operations. [1] [2]Minor Dependency Updates:
using System.Threading.Tasks;directives in affected files to support the new async signatures. [1] [2]