Added IStreamStack for debugging and configurable buffer management. …#930
Added IStreamStack for debugging and configurable buffer management. …#930adamhathcock merged 9 commits intoadamhathcock:masterfrom Nanook:feature/StreamStack
Conversation
…Added SharpCompressStream to consolodate streams to help simplify debugging. All unit tests passing.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces the IStreamStack interface to provide a unified stream hierarchy with configurable buffer management and enhanced debugging capabilities for SharpCompress. The changes consolidate stream functionality from NonDisposingStream, RewindableStream, and CountingWritableSubStream into the new SharpCompressStream class.
Key Changes
- Introduced IStreamStack interface for universal stream hierarchy management with flexible buffer control
- Replaced legacy stream classes with SharpCompressStream providing over-read protection and position tracking
- Added DEBUG_STREAMS conditional compilation for comprehensive debugging with zero release performance impact
Reviewed Changes
Copilot reviewed 84 out of 84 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/SharpCompress/IO/IStreamStack.cs |
Core interface definition with buffer management and debugging extensions |
src/SharpCompress/IO/SharpCompressStream.cs |
Unified stream implementation replacing multiple legacy stream classes |
src/SharpCompress/IO/NonDisposingStream.cs |
Removed legacy stream class replaced by SharpCompressStream |
tests/SharpCompress.Test/Streams/SharpCompressStreamTest.cs |
New test suite for buffer management functionality |
| Multiple stream implementations | Updated to implement IStreamStack interface for hierarchy participation |
| throw new NotSupportedException(); | ||
| } | ||
|
|
||
| public override void WriteByte(byte value) |
There was a problem hiding this comment.
The WriteByte method does not handle buffering consistently with the Read method. When buffering is enabled, writes should either be buffered or the buffer should be invalidated to maintain data integrity.
There was a problem hiding this comment.
The Stream/Buffer would never be used for both purposes in the same instance. This was taken from RewindableStream.
There was a problem hiding this comment.
Smells wrong but if it's previous behavior then all good
There was a problem hiding this comment.
Currently buffering isn't really used for writing. There's no ability to pass buffer sizes for writing currently either. This is planned to be added to perhaps aid LZMA2 block writing. I was conscious about the scope of the change and keeping it reviewable, getting a stable commit to build on with a bit of confidence :-)
| ++_internalPosition; | ||
| } | ||
|
|
||
| public override void Write(byte[] buffer, int offset, int count) | ||
| { | ||
| Stream.Write(buffer, offset, count); | ||
| _internalPosition += count; |
There was a problem hiding this comment.
The Write method does not handle buffering consistently with the Read method. When buffering is enabled, writes should either be buffered or the buffer should be invalidated to maintain data integrity.
| ++_internalPosition; | |
| } | |
| public override void Write(byte[] buffer, int offset, int count) | |
| { | |
| Stream.Write(buffer, offset, count); | |
| _internalPosition += count; | |
| ++_internalPosition; | |
| _bufferedLength = 0; // Invalidate the buffer | |
| _bufferPosition = 0; | |
| } | |
| public override void Write(byte[] buffer, int offset, int count) | |
| { | |
| Stream.Write(buffer, offset, count); | |
| _internalPosition += count; | |
| _bufferedLength = 0; // Invalidate the buffer | |
| _bufferPosition = 0; |
There was a problem hiding this comment.
Same as previous WriteByte issue.
| private byte[]? _buffer; | ||
| private int _bufferPosition; | ||
| private int _bufferedLength; | ||
| private bool _bufferingEnabled; |
There was a problem hiding this comment.
The buffer state variables (_bufferPosition, _bufferedLength) are not validated for consistency during buffer operations, which could lead to corrupt buffer states.
| private bool _bufferingEnabled; | |
| private bool _bufferingEnabled; | |
| private void ValidateBufferState() | |
| { | |
| if (_bufferPosition < 0 || _bufferPosition > _bufferedLength) | |
| { | |
| throw new InvalidOperationException("Buffer state is inconsistent: _bufferPosition is out of range."); | |
| } | |
| } |
src/SharpCompress/IO/IStreamStack.cs
Outdated
There was a problem hiding this comment.
The Rewind method decrements BufferPosition without validating that the result is non-negative, which could result in an invalid buffer state.
There was a problem hiding this comment.
The rented buffer from ArrayPool is never returned, causing a resource leak. ArrayPool.Return should be called in a finally block or using a proper disposal pattern.
| var buffer = ArrayPool<byte>.Shared.Rent(1024); |
There was a problem hiding this comment.
Incorrect type passed to DebugConstruct - should be typeof(LzmaStream), not typeof(LZipStream).
| this.DebugConstruct(typeof(LZipStream)); | |
| this.DebugConstruct(typeof(LzmaStream)); |
There was a problem hiding this comment.
Incorrect type passed to DebugConstruct - should be typeof(LzmaStream), not typeof(LZipStream).
| this.DebugConstruct(typeof(LZipStream)); | |
| this.DebugConstruct(typeof(LzmaStream)); |
There was a problem hiding this comment.
Incorrect type passed to DebugDispose - should be typeof(LzmaStream), not typeof(LZipStream).
| this.DebugDispose(typeof(LZipStream)); | |
| this.DebugDispose(typeof(LzmaStream)); |
There was a problem hiding this comment.
Hard-coded magic number 4 should be replaced with a named constant to improve maintainability and explain its purpose (likely sizeof(uint) for header bytes).
| ((IStreamStack)rewindableStream).Rewind(4); | |
| ((IStreamStack)rewindableStream).Rewind(UInt32Size); |
| // Fill buffer if needed | ||
| if (_bufferedLength == 0) | ||
| { | ||
| _bufferedLength = Stream.Read(_buffer!, 0, _bufferSize); |
There was a problem hiding this comment.
[nitpick] The buffer refill logic reads the full buffer size even when only a small amount is needed, which could impact performance for large buffers with small reads.
| _bufferedLength = Stream.Read(_buffer!, 0, _bufferSize); | |
| int bytesToRead = Math.Min(_bufferSize, count); | |
| _bufferedLength = Stream.Read(_buffer!, 0, bytesToRead); |
There was a problem hiding this comment.
It's supposed to attempt to read the size of the buffer. That change makes lots of things fail.
|
This is exactly the type of thing I've been hoping for: a way to bring things closer and that it makes sense. Thank you! I'm gonna sit on it for a couple days to read and think but if the tests pass and there's little to no breaking changes (haven't read yet) then I don't see why not. |
|
...and that is the exact response I was hoping for too. Not a rejection because it looks too intrusive and not straight out acceptance :-). I'm cautious about it, but as you say "it makes sense" to attempt it. I'm happy to discuss it and rework parts of it (or for anyone to). You can always reach me on discord too if you want a more productive chat. Cheers. |
|
I noticed that SetPosition was mis-spelled as SetPostion and realised that I'd corrected this already. I'd accidentally merged the commit from getting it working and not the commit after finessing it a bit. You'll notice that StackSeek now does not have a hard coded 0 for the position when called by the various factory classes that use it too. |
2 participants
IStreamStack Buffering Enhancement
This pull request introduces a comprehensive buffer management stack that simplifies stream over-reading in compression formats, adds optional stream hierarchy debugging with position tracking (no release performance impact), and establishes a clean foundation for future stream consolidation and simplification.
Warning: This is a fundamental change and needs to be looked over carefully. It was born from having a lot of difficulty debugging stream positions and rewinding. A lot of the files changed are to add a StreamStack interface, other changes are to cater for SharpCompressStream and buffer usage. All unit tests are passing with minimal or zero change.
🔧 Core Changes
1. Enhanced IStreamStack Interface
IStreamStackBufferSize,BufferPosition,DefaultBufferSizeproperties for systematic buffer placementInstanceIdfor comprehensive stream tracking (DEBUG_STREAMS only - zero release impact)BaseStream()method maintains hierarchy navigation regardless of implementation type2. Unified SharpCompressStream
Consolidates the functionality of NonDisposingStream, RewindableStream and CountingWritableSubStream into a single implementation. Future refactors could simplify things further. SharpCompressStream provides:
3. 🔍 DEBUG_STREAMS C# define for Debugging Visualization
Activated for .NET 8.0 Debug builds only, provides complete stream hierarchy tracking with zero performance impact on release builds:
Key Insights from This Debug Output:
SharpCompressStream#1position increment (Px0→Px25→Px28→Px5e→Px62) as it processes ZIP entries etcBx10000(64KB configurable buffer) strategically placed at base level for over-reading protectionFormat:
StreamType#InstanceId[Position:BufferSize:DefaultBufferSize]📈 Benefits
Changes
Strategic Foundation
🧪 Usage
Result: Compression streams can over-read safely into managed buffers, then rewind to exact boundaries, allowing base streams to resume correctly for subsequent processing.
This enhancement introduces a comprehensive buffer management stack that solves fundamental stream processing issues while providing optional debugging capabilities with zero release performance impact. The flexible interface-based design allows any object to participate in stream hierarchies, creating a solid foundation for future consolidation phases.