Change interfaces to be consistent for new Async paths (definitely breaks things)#1128
Change interfaces to be consistent for new Async paths (definitely breaks things)#1128adamhathcock merged 38 commits intomasterfrom
Conversation
Change return types from ValueTask<T> to direct interface types (IAsyncArchive, IAsyncReader, IWriter) for wrapper methods that don't perform async work. This eliminates unnecessary async state machine allocations while maintaining the same public API behavior. Changes: - Interface definitions: Updated IArchiveFactory, IMultiArchiveFactory, IReaderFactory, IWriterFactory - Concrete factories: Updated archive factories (Zip, Tar, Rar, GZip, SevenZip) and reader-only factories (Ace, Arc, Arj) - Static factory methods: Updated ReaderFactory, ArchiveFactory, WriterFactory to use new signatures - Archive classes: Updated static OpenAsync methods in ZipArchive, TarArchive, RarArchive, SevenZipArchive, GZipArchive - Supporting changes: Updated Factory.cs and async polyfills Performance benefit: Reduced GC pressure by eliminating unnecessary state machine overhead for non-async wrapper methods.
…y.OpenAsync and use IAsyncReader - Removed 'await' keyword before ReaderFactory.OpenAsync() calls since the method returns IAsyncReader directly (not Task) - Changed ZipReader.Open() to ReaderFactory.OpenAsync() in Zip64AsyncTests.ReadForwardOnlyAsync() - Changed TarReader.Open() to ReaderFactory.OpenAsync() in TarReaderAsyncTests.Tar_BZip2_Entry_Stream_Async() - Fixed EntryStream disposal from 'await using' to 'using' since EntryStream doesn't implement IAsyncDisposable - These changes fix compilation errors where async methods were being called on IReader (synchronous) instead of IAsyncReader (asynchronous)
| writerOptions.ArchiveEncoding.Default = encoding ?? Encoding.Default; | ||
|
|
||
| using var writer = WriterFactory.Open(stream, _type, writerOptions); | ||
| using var writer = WriterFactory.OpenAsync(stream, _type, writerOptions); |
There was a problem hiding this comment.
The WriterFactory.OpenAsync methods now return IAsyncWriter instead of ValueTask, but they're being used without await. This is inconsistent with the documented async pattern. The tests are calling WriterFactory.OpenAsync() synchronously when they should either await it or use WriterFactory.Open() for synchronous operations.
| readerOptions.ArchiveEncoding.Default = encoding ?? Encoding.Default; | ||
|
|
||
| await using var reader = await ReaderFactory.OpenAsync( | ||
| await using var reader = ReaderFactory.OpenAsync( |
There was a problem hiding this comment.
ReaderFactory.OpenAsync() is being called without await, but the method returns IAsyncReader synchronously (not a Task). This is inconsistent with typical async method patterns. The change makes OpenAsync effectively synchronous, which is confusing naming. Either the method should be renamed to Open() for sync usage, or it should return a Task/ValueTask and be awaited.
| { | ||
| using Stream stream = File.OpenWrite(Path.Combine(SCRATCH_FILES_PATH, "Tar.tar.gz")); | ||
| using var writer = WriterFactory.Open(stream, ArchiveType.GZip, CompressionType.BZip2); | ||
| using var writer = WriterFactory.Open( |
There was a problem hiding this comment.
The Open() method is being used instead of the async equivalent, but this is in an async test method. For consistency with the async test pattern and to properly test async paths, this should use OpenAsync instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 133 out of 134 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static async IAsyncEnumerable<TResult> CastAsync<TResult>( | ||
| this IAsyncEnumerable<object?> source | ||
| ) |
There was a problem hiding this comment.
The method is named CastAsync but should follow the extension naming pattern used elsewhere in the file (e.g., it's defined outside the extension blocks). Consider moving this inside an extension block or renaming for consistency.
|
|
||
| private static bool SignatureMatch(Stream stream) | ||
| { | ||
| var reader = new BinaryReader(stream); |
There was a problem hiding this comment.
Disposable 'BinaryReader' is created but not disposed.
| && Enum.IsDefined(typeof(EntryType), tarHeader.EntryType); | ||
| return readSucceeded || isEmptyArchive; | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Poor error handling: empty catch block.
|
This is known to be imperfect for asynchronous work but it's on it's way |
1 participant
API thinking here: #1129
This pull request introduces several improvements and clarifications to the SharpCompress codebase and documentation, focusing on enhancing async support, updating documentation for modern usage, and cleaning up code and configuration files. The most significant changes are grouped and summarized below.
Async API Enhancements and Consistency:
AbstractArchiveby using explicit async enumerator methods for casting and updating aggregate operations to use their async variants (e.g.,AggregateAsync,AllAsync). Also addedTotalUncompressedSizeAsyncand clarifiedIsEncryptedAsyncreturn type. [1] [2]ArchiveFactoryasync methods to remove unnecessaryConfigureAwait(false), streamline calls, and eliminate redundant XML documentation comments, making the async API more idiomatic and easier to maintain. [1] [2] [3] [4] [5] [6] [7] [8]AbstractWritableArchiveto implement the newIWritableAsyncArchiveinterface and unified interface method implementations underIWritableArchiveCommon, improving consistency for writable archives. [1] [2]Documentation Updates and Modernization:
docs/API.mdto reflect modern async usage patterns, clarify types (IEntry→IArchiveEntry), provide more accurate code samples, and expand on supported compression and archive types. Also updated extraction and writer usage to reflect best practices and new APIs. [1] [2] [3] [4] [5] [6] [7]Configuration and Project Maintenance:
Microsoft.NET.ILLink.Taskpackage to central package management and enabledCentralPackageTransitivePinningEnabledin build properties to improve dependency management. [1] [2].editorconfigrule that set CS1998 (async method lacks 'await') as an error, likely to reduce noise for intentionally synchronous async methods..gitignoreand remove obsolete documentation files, reflecting a cleanup of project structure.Minor Documentation and Typo Fixes:
TotalUncompressSize→TotalUncompressedSize) for clarity and consistency.AGENTS.md.