Conversation
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return new SeekableSharpCompressStream(underlying); | ||
| } | ||
| // Non-seekable underlying stream - wrap with rolling buffer | ||
| return new SharpCompressStream(underlying, true, false, rewindableBufferSize); |
There was a problem hiding this comment.
WARNING: Inconsistent LeaveStreamOpen behavior
When unwrapping a passthrough SharpCompressStream with a non-seekable underlying stream, the new stream is created with leaveStreamOpen: true. However, the original passthrough stream's LeaveStreamOpen property is not being respected. If the original stream had LeaveStreamOpen = false, this change in behavior could cause the underlying stream to be left open when it should have been disposed.
Consider preserving the original stream's LeaveStreamOpen value from sharpCompressStream.LeaveStreamOpen.
| if (_ringBuffer is null) | ||
| { | ||
| _ringBuffer = new RingBuffer(DefaultRollingBufferSize); | ||
| _ringBuffer = new RingBuffer(Constants.BufferSize); |
There was a problem hiding this comment.
WARNING: Inconsistent buffer size usage
StartRecording() uses Constants.BufferSize (81920) when creating the ring buffer, but the Create() method uses Constants.RewindableBufferSize (also 81920 by default) for the same purpose. While these happen to have the same default value, they are semantically different constants.
If a user sets ReaderOptions.RewindableBufferSize to a custom value, StartRecording() will still use Constants.BufferSize instead of respecting the configured buffer size. This could lead to unexpected behavior where the recording buffer size differs from what the user configured.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This PR refactors the SharpCompress stream handling with the following improvements: Changes Overview
Code Quality
Files Reviewed (9 files)
|
There was a problem hiding this comment.
Pull request overview
This pull request refactors the stream creation and management logic in SharpCompress, replacing the EnsureSeekable method with a new Create method and standardizing internal field naming from _underlyingStream to _stream.
Changes:
- Introduced
SharpCompressStream.Createmethod to replaceEnsureSeekable, providing unified stream wrapping logic - Renamed internal stream field from
_underlyingStreamto_streaminSeekableSharpCompressStreamclasses - Made
LeaveStreamOpenproperty read-only inSharpCompressStreambase class - Updated all call sites to use the new
Createmethod with named parameter syntax
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SharpCompress/IO/SharpCompressStream.Create.cs | New file containing the Create and CreateNonDisposing factory methods |
| src/SharpCompress/IO/SharpCompressStream.cs | Removed EnsureSeekable method, made LeaveStreamOpen read-only, updated exception messages |
| src/SharpCompress/IO/SeekableSharpCompressStream.cs | Renamed _underlyingStream to _stream, removed NullStream class, updated constructor |
| src/SharpCompress/IO/SeekableSharpCompressStream.Async.cs | Updated all references from _underlyingStream to _stream |
| src/SharpCompress/Readers/Tar/TarReader.cs | Changed from EnsureSeekable to Create with named parameter |
| src/SharpCompress/Readers/ReaderFactory.cs | Changed from EnsureSeekable to Create with named parameter |
| src/SharpCompress/Readers/ReaderFactory.Async.cs | Changed from EnsureSeekable to Create with named parameter |
| src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs | Changed from EnsureSeekable to Create, updated comment |
| src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs | Changed from EnsureSeekable to Create, updated comment |
Comments suppressed due to low confidence (2)
src/SharpCompress/IO/SharpCompressStream.Create.cs:60
- There's a spacing issue in the code. The comma should be followed by a space:
false,falseshould befalse, false. This violates standard C# formatting conventions.
src/SharpCompress/IO/SharpCompressStream.Create.cs:60 - The parameter passed here is
bufferSize(which can be null), but earlier in the method at line 22,rewindableBufferSizeis calculated as the fallback value. This should berewindableBufferSizeto ensure consistency with how the buffer size is handled elsewhere in the Create method.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return new SeekableSharpCompressStream(underlying); | ||
| } | ||
| // Non-seekable underlying stream - wrap with rolling buffer | ||
| return new SharpCompressStream(underlying, true, false, rewindableBufferSize); |
There was a problem hiding this comment.
The comment states "Create SeekableSharpCompressStream that preserves LeaveStreamOpen", but the code doesn't actually preserve the LeaveStreamOpen value from the passthrough stream. The newly created SeekableSharpCompressStream will have LeaveStreamOpen default to false. If preservation is intended, the code should set LeaveStreamOpen = sharpCompressStream.LeaveStreamOpen after creating the instance.
| return new SeekableSharpCompressStream(underlying); | |
| } | |
| // Non-seekable underlying stream - wrap with rolling buffer | |
| return new SharpCompressStream(underlying, true, false, rewindableBufferSize); | |
| var seekable = new SeekableSharpCompressStream(underlying) | |
| { | |
| LeaveStreamOpen = sharpCompressStream.LeaveStreamOpen | |
| }; | |
| return seekable; | |
| } | |
| // Non-seekable underlying stream - wrap with rolling buffer | |
| return new SharpCompressStream(underlying, sharpCompressStream.LeaveStreamOpen, false, rewindableBufferSize); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return underlying; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent buffer size handling: Line 34 uses rewindableBufferSize (which is never null and defaults to Constants.RewindableBufferSize), but line 57 uses bufferSize which can be null. This means that unwrapping a passthrough stream will always create a buffer, while creating from a raw non-seekable stream might not create a buffer if bufferSize is null. This inconsistency could lead to unexpected behavior. Consider using rewindableBufferSize here for consistency.
| if (underlying.CanSeek) | ||
| { | ||
| // Create SeekableSharpCompressStream that preserves LeaveStreamOpen | ||
| return new SeekableSharpCompressStream( |
There was a problem hiding this comment.
Inconsistent LeaveStreamOpen behavior: When creating a SeekableSharpCompressStream from an unwrapped passthrough stream (line 31) or a raw seekable stream (line 52), the LeaveStreamOpen property is not explicitly set, so it defaults to false (the default value for bool). However, the comment on line 30 says "preserves LeaveStreamOpen", which is misleading since the original passthrough stream had LeaveStreamOpen=true. This means the underlying stream will be disposed when it shouldn't be, breaking the intended behavior. Consider passing leaveStreamOpen to the SeekableSharpCompressStream constructor.
| ) | ||
| { | ||
| this.stream = stream; | ||
| LeaveStreamOpen = leaveStreamOpen; |
There was a problem hiding this comment.
Cannot assign to LeaveStreamOpen because it's declared as a get-only auto-property on line 35. Get-only auto-properties can only be set in a constructor body of the same class where they're declared, not in a different constructor. This code will not compile. The property needs either a setter (e.g., public virtual bool LeaveStreamOpen { get; protected set; }) or should be declared with init (e.g., public virtual bool LeaveStreamOpen { get; init; }).
| private bool _isDisposed; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets whether to leave the underlying stream open when disposed. |
There was a problem hiding this comment.
Documentation mismatch: The comment says "Gets or sets" but the property is get-only. Since the property cannot be set after construction, the documentation should say "Gets whether to leave the underlying stream open when disposed."
| /// Gets or sets whether to leave the underlying stream open when disposed. | |
| /// Gets whether to leave the underlying stream open when disposed. |
| @@ -33,44 +33,45 @@ public SeekableSharpCompressStream(Stream stream) | |||
| { | |||
| throw new ArgumentException("Stream must be seekable", nameof(stream)); | |||
| } | |||
| _underlyingStream = stream; | |||
|
|
|||
| LeaveStreamOpen = leaveStreamOpen; | |||
There was a problem hiding this comment.
The SeekableSharpCompressStream constructor should accept a leaveStreamOpen parameter to support the use case in Create() where LeaveStreamOpen behavior needs to be preserved when unwrapping passthrough streams. Currently, there's no way to control whether the underlying stream is disposed, which breaks the intended behavior described in the comment on line 30.
| false, | ||
| rewindableBufferSize | ||
| ); | ||
| } | ||
| // Not passthrough - return as-is | ||
| return sharpCompressStream; | ||
| } |
There was a problem hiding this comment.
Inconsistent handling of passthrough streams: When the stream is directly a passthrough SharpCompressStream (line 24), it's unwrapped and properly wrapped. However, when the stream is an IStreamStack containing a passthrough SharpCompressStream (line 43-46), the passthrough stream is returned as-is without unwrapping. This creates inconsistent behavior depending on how the stream is wrapped. Consider checking if the underlying stream is a passthrough and handling it consistently with the direct case.
1 participant
This pull request refactors how
SharpCompressStreamwrappers are created and managed, replacing the oldEnsureSeekablemethod with a new, more flexibleCreatemethod. It also standardizes the internal stream field naming and updates all usages to leverage the new creation logic. These changes improve clarity, consistency, and future extensibility for stream handling in the library.Stream creation and management refactor:
SharpCompressStream.Createmethod, which replacesEnsureSeekableand provides a unified way to wrap streams, handling passthrough, seekable, and buffered cases. The oldEnsureSeekablelogic was removed. (src/SharpCompress/IO/SharpCompressStream.Create.cs,src/SharpCompress/IO/SharpCompressStream.cs) [1] [2]EnsureSeekableto use the newCreatemethod, including in ZIP and TAR header factories and reader factories. (src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs,src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs,src/SharpCompress/Readers/ReaderFactory.Async.cs,src/SharpCompress/Readers/ReaderFactory.cs,src/SharpCompress/Readers/Tar/TarReader.cs) [1] [2] [3] [4] [5]Internal field and property standardization:
_underlyingStreamto_streamacross all relevant files, and updated all property and method references accordingly for consistency. (src/SharpCompress/IO/SeekableSharpCompressStream.cs,src/SharpCompress/IO/SeekableSharpCompressStream.Async.cs) [1] [2] [3] [4] [5]NullStreamclass. (src/SharpCompress/IO/SeekableSharpCompressStream.cs) [1] [2]Buffering and recording improvements:
DefaultRollingBufferSizewithConstants.BufferSizeand ensuring consistent buffer allocation for rolling buffers. (src/SharpCompress/IO/SharpCompressStream.cs)Createmethod instead of the deprecatedEnsureSeekable. (src/SharpCompress/IO/SharpCompressStream.cs) [1] [2] [3]Minor logic and property fixes:
CanSeekandCanWriteproperties to better handle passthrough and seekable scenarios. (src/SharpCompress/IO/SharpCompressStream.cs)LeaveStreamOpento be read-only and set in the constructor. (src/SharpCompress/IO/SharpCompressStream.cs)Let me know if you want a deeper dive into how the new
Createmethod works or why these changes improve the library!