zstd: Fix extra CRC written with multiple Close calls #1017
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request enhance error handling and control flow in the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
zstd/zstd.go (1)
91-93: LGTM! Consider adding a comment about usage.The addition of
ErrEncoderClosedis well-placed and consistent with the existing error declarations. It complementsErrDecoderClosedand aligns with the PR objectives.Consider adding a brief comment above the error declaration to provide context on when and where this error is used, similar to the comment for
ErrDecoderClosed. This would enhance code readability and maintainability. For example:// ErrEncoderClosed will be returned if the Encoder is used after // Close has been called. ErrEncoderClosed = errors.New("encoder used after Close")zstd/encoder.go (1)
421-421: LGTM: Improved error handling for idempotent Close() method.The changes effectively address the issue of multiple
Close()calls writing duplicate CRC checksums. The method is now idempotent, aligning with the behavior of otherio.WriteCloserimplementations.A minor suggestion for improvement:
Consider using a custom error variable (e.g.,ErrEncoderClosed) instead ofErrDecoderClosedfor better clarity and consistency with the encoder context.If you agree, you could implement this change as follows:
-if s.encoder == nil || errors.Is(s.err, ErrDecoderClosed) { +if s.encoder == nil || errors.Is(s.err, ErrEncoderClosed) { return nil } // ... (existing code) if s.err == nil { - s.err = ErrDecoderClosed + s.err = ErrEncoderClosed return nil }Don't forget to declare
ErrEncoderClosedat the package level:var ErrEncoderClosed = errors.New("encoder closed")Also applies to: 463-467
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- zstd/encoder.go (3 hunks)
- zstd/encoder_test.go (1 hunks)
- zstd/zstd.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
zstd/encoder.go (1)
9-9: LGTM: New import for error handling.The addition of the
errorspackage import is appropriate for the new error handling in theClose()method.zstd/encoder_test.go (2)
281-284: LGTM: Testing multiple Close() callsThis addition improves the test coverage by verifying the behavior of calling
Close()multiple times on the encoder. It aligns well with the PR objective of fixing issues related to multipleClose()calls.
291-291: Improved error loggingThe modification enhances error reporting by including the expected and actual lengths of the decoded data. This additional information will be valuable for debugging in case of test failures.
Fixes #1016
Summary by CodeRabbit
ErrEncoderClosed, for improved error handling when using the Encoder after closure.Write,Flush, andClosemethods of the Encoder to manage specific error states after closure.