Some clean up post-async merging#1184
Conversation
# Conflicts: # src/SharpCompress/Common/EntryStream.cs
There was a problem hiding this comment.
Pull request overview
This pull request modernizes the codebase by adopting file-scoped namespaces, consolidating stream wrapper functionality, and improving async stream handling. The changes refactor stream management to use a unified SharpCompressStream wrapper instead of the previous NonDisposingStream and RewindableStream classes, while also removing the useSyncOverAsyncDispose parameter in favor of a centralized utility method.
Changes:
- Converted multiple files to use file-scoped namespace declarations for reduced indentation
- Replaced
NonDisposingStreamwithSharpCompressStream.CreateNonDisposing()throughout the codebase - Renamed
RewindableStreamtoSharpCompressStreamandSeekableRewindableStreamtoSeekableSharpCompressStream - Removed
useSyncOverAsyncDisposeparameter from multiple methods, replacing withUtility.UseSyncOverAsyncDispose() - Enhanced async APIs with proper conditional compilation for legacy vs. modern .NET
- Removed DEBUG_STREAMS conditional compilation directives
Reviewed changes
Copilot reviewed 159 out of 159 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| Multiple test files | Updated to use SharpCompressStream.CreateNonDisposing() and renamed class references |
| Writer classes (Zip, Tar, GZip) | Replaced NonDisposingStream with SharpCompressStream.CreateNonDisposing() |
| Reader classes (Rar, Arj, Arc, Ace) | Removed useSyncOverAsyncDispose parameter and updated to file-scoped namespaces |
| Common classes | Renamed stream classes and updated references throughout |
| Archive entry classes | Updated stream handling for writable entries |
| Factory classes | Updated to use SharpCompressStream instead of RewindableStream |
| Compressor classes | Removed DEBUG_STREAMS directives |
| Namespace files (Arj, Arc, Ace) | Converted to file-scoped namespaces |
Comments suppressed due to low confidence (1)
src/SharpCompress/IO/SharpCompressStream.cs:220
- The expression 'A ? B : true' can be simplified to '!A || B'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (true) | ||
| { | ||
| var reader = new BinaryReader(rewindableStream); | ||
| var reader = new BinaryReader(sharpCompressStream); |
There was a problem hiding this comment.
Disposable 'BinaryReader' is created but not disposed.
| throw new NotSupportedException(); | ||
| private Stream BuildDecodedStream() | ||
| { | ||
| var binaryReader = new BinaryReader(_stream, Encoding.Default, leaveOpen: true); |
There was a problem hiding this comment.
Disposable 'BinaryReader' is created but not disposed.
| rewindableStream.Rewind(); | ||
| var testStream = new GZipStream(rewindableStream, CompressionMode.Decompress); | ||
| sharpCompressStream.Rewind(); | ||
| var testStream = new GZipStream(sharpCompressStream, CompressionMode.Decompress); |
There was a problem hiding this comment.
Disposable 'GZipStream' is created but not disposed.
| #region Instance Fields | ||
|
|
||
| private Stream baseInputStream; | ||
| private Stream baseInputStream; |
There was a problem hiding this comment.
Field 'baseInputStream' can be 'readonly'.
| internal class BitStream | ||
| private const int EOF = 1234; | ||
| private byte[] _src; | ||
| private int _srcLen; |
There was a problem hiding this comment.
Field '_srcLen' can be 'readonly'.
| private byte[] _src; | ||
| private int _srcLen; | ||
| private int _byteIdx; | ||
| private int _bitIdx; |
There was a problem hiding this comment.
Field '_bitIdx' can be 'readonly'.
| { | ||
| internal class BitStream | ||
| private const int EOF = 1234; | ||
| private byte[] _src; |
There was a problem hiding this comment.
Field '_src' can be 'readonly'.
| private int _bitIdx; | ||
| private int _bitsLeft; | ||
| private ulong _bitBuffer; | ||
| private static uint[] _maskBits = new uint[17] |
There was a problem hiding this comment.
Field '_maskBits' can be 'readonly'.
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| _isDisposed = true; | ||
| if (!LeaveStreamOpen) | ||
| { | ||
| _underlyingStream.Dispose(); |
There was a problem hiding this comment.
WARNING: Inconsistent async dispose pattern - synchronous dispose on async path
The DisposeAsync() method calls _underlyingStream.Dispose() (synchronous) instead of await _underlyingStream.DisposeAsync(). This could block the async flow and cause deadlocks in certain scenarios.
| _underlyingStream.Dispose(); | |
| await _underlyingStream.DisposeAsync(); |
| await stream.DisposeAsync(); | ||
| if (!LeaveStreamOpen) | ||
| { | ||
| await stream.DisposeAsync(); |
There was a problem hiding this comment.
WARNING: Missing .ConfigureAwait(false) on async dispose
The DisposeAsync() method calls await stream.DisposeAsync() without .ConfigureAwait(false). This could cause deadlocks in UI or ASP.NET Classic contexts.
| await stream.DisposeAsync(); | |
| await stream.DisposeAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 177 out of 177 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| offset += 2 + commentLength; | ||
| } | ||
| ushort commentLength = BitConverter.ToUInt16(headerData, offset); | ||
| offset += 2 + commentLength; |
|
|
||
| var version = reader.ReadUInt16(); | ||
| var flags = (HeaderFlags)reader.ReadUInt16(); | ||
| var compressionMethod = (ZipCompressionMethod)reader.ReadUInt16(); |
There was a problem hiding this comment.
This assignment to compressionMethod is useless, since its value is never read.
| var version = reader.ReadUInt16(); | ||
| var flags = (HeaderFlags)reader.ReadUInt16(); | ||
| var compressionMethod = (ZipCompressionMethod)reader.ReadUInt16(); | ||
| var lastModifiedDate = reader.ReadUInt16(); |
There was a problem hiding this comment.
This assignment to lastModifiedDate is useless, since its value is never read.
| var flags = (HeaderFlags)reader.ReadUInt16(); | ||
| var compressionMethod = (ZipCompressionMethod)reader.ReadUInt16(); | ||
| var lastModifiedDate = reader.ReadUInt16(); | ||
| var lastModifiedTime = reader.ReadUInt16(); |
There was a problem hiding this comment.
This assignment to lastModifiedTime is useless, since its value is never read.
| && local_header.CompressedSize == 0 | ||
| && local_header.UncompressedSize == 0 | ||
| && local_header.Crc == 0 | ||
| && local_header.IsDirectory == false |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
|
|
||
| public override bool CanRead => true; | ||
|
|
||
| public override bool CanSeek => _isPassthrough ? stream.CanSeek : true; |
There was a problem hiding this comment.
The expression 'A ? B : true' can be simplified to '!A || B'.
| var rewindableStream = RewindableStream.EnsureSeekable(stream); | ||
| while (true) | ||
| var sharpCompressStream = SharpCompressStream.EnsureSeekable(stream); | ||
| var reader = new BinaryReader( |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (X files)
|
1 participant
This pull request refactors several files to adopt file-scoped namespaces for improved readability and consistency, and updates stream handling logic for archive entries to use a unified stream wrapper. Additionally, it enhances asynchronous extraction APIs and simplifies code in some archive format implementations.
Namespace Refactoring:
SharpCompress.Common.AceandSharpCompress.Common.Arcnamespaces to use file-scoped namespace declarations, reducing indentation and boilerplate. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]Stream Handling Improvements:
NonDisposingStreamwithSharpCompressStream.CreateNonDisposingin archive entry stream methods for GZip, Tar, and Zip writable archive entries, ensuring consistent stream management. [1] [2] [3]Async API and Extraction Enhancements:
TarArchiveEntry.OpenEntryStreamAsyncto use an actual asynchronous stream retrieval, improving correctness for async extraction.WriteToAsyncto use conditional compilation for legacy and modern .NET async stream disposal, ensuring compatibility across frameworks.SevenZip Archive Code Simplification:
SevenZipArchive.GetEntryStream: removed the unuseduseSyncOverAsyncDisposeparameter and cleaned up related calls. [1] [2]Arc Archive Async Stream Handling:
ArcFilePart.Async.csby reducing nesting and simplifying the construction of compression streams. [1] [2]These changes collectively modernize the codebase, improve maintainability, and enhance compatibility with newer .NET features.