Rewrite ChunkedMemoryStream #6
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| int offset = 0; | ||
| int count = buffer.Length; | ||
| while (count > 0) | ||
| while (bytesToRead != 0 && this.currentChunk != this.memoryChunkBuffer.Length) |
There was a problem hiding this comment.
logic: potential infinite loop if currentChunk reaches memoryChunkBuffer.Length but bytesToRead is still non-zero
| int copyCount = Math.Min(count, chunkSize - this.writeOffset); | ||
| buffer.Slice(offset, copyCount).CopyTo(chunkBuffer[this.writeOffset..]); | ||
| // Write n bytes to the current chunk | ||
| buffer.Slice(offset, n).CopyTo(chunk.Buffer.Slice(this.currentChunkIndex, n)); |
There was a problem hiding this comment.
logic: Buffer.Slice() may throw if currentChunkIndex + n exceeds the buffer length
Comment on lines
+366
to
+371
| // If the new position is greater than the length of the stream, set the position to the end of the stream | ||
| if (offset > 0 && offset >= this.memoryChunkBuffer.Length) | ||
| { | ||
| if (this.readOffset == chunkSize) | ||
| { | ||
| // Exit if no more chunks are currently available | ||
| if (this.readChunk.Next is null) | ||
| { | ||
| break; | ||
| } | ||
| this.currentChunk = this.memoryChunkBuffer.ChunkCount - 1; | ||
| this.currentChunkIndex = this.memoryChunkBuffer[this.currentChunk].Length - 1; | ||
| return; |
There was a problem hiding this comment.
logic: setting position to Length-1 when seeking beyond stream end differs from standard Stream behavior which allows seeking beyond end
Comment on lines
+376
to
387
| while (offset != 0) | ||
| { | ||
| int chunkLength = this.memoryChunkBuffer[currentChunkIndex].Length; | ||
| if (offset < chunkLength) | ||
| { | ||
| // Found the correct chunk and the corresponding index | ||
| break; | ||
| } | ||
|
|
||
| int writeCount = chunkSize - this.readOffset; | ||
| stream.Write(chunkBuffer.GetSpan(), this.readOffset, writeCount); | ||
| this.readOffset = chunkSize; | ||
| offset -= chunkLength; | ||
| currentChunkIndex++; | ||
| } |
There was a problem hiding this comment.
logic: will throw IndexOutOfRangeException if offset is larger than total chunk lengths
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites
Description
Fixes SixLabors#2806
ChunkedMemoryStream contained multiple writing bugs and was too costly to fix/maintain relative to performance benefits so I'm just ditching it.
Greptile Summary
Complete rewrite of ChunkedMemoryStream to fix critical bugs and improve non-seekable stream handling in ImageSharp's encoding pipeline.
ChunkedMemoryStreamto use a simpler List-based chunk management system instead of linked listsImageEncoderto properly use chunked memory stream for non-seekable stream encodingawait usinginEncodeWithSeekableStreamAsyncNonSeekableStreamto correctly delegate write operations to underlying stream