gzhttp: Add zstandard to server handler wrapper#1121
Conversation
Both gzip and zstd compression are enabled by default. When a client supports both, zstd is preferred due to its better compression ratio and speed. Zstd compression is enabled by default alongside gzip. When the client supports both, zstd is preferred because it typically offers better compression ratios and faster decompression. The server uses `Accept-Encoding` header negotiation to select the best encoding: - If client only accepts `gzip` → response is gzip compressed - If client only accepts `zstd` → response is zstd compressed - If client accepts both with equal qvalues → zstd is used (configurable) - If client specifies qvalues (e.g., `gzip;q=1.0, zstd;q=0.5`) → higher qvalue wins
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gzhttp/compress.go (1)
257-261: ETag suffix replacement may have no effect for custom suffixes.The
strings.Replaceonly transforms the suffix if it contains "gzip". If a user specifies a custom suffix without "gzip" (e.g.,SuffixETag("-compressed")), both gzip and zstd responses will use the same ETag suffix, potentially causing cache conflicts.Consider documenting this behavior or providing an encoding-specific suffix option.
📝 Possible alternative approach
suffix := w.suffixETag if w.enc == encodingZstd { - suffix = strings.Replace(suffix, "gzip", "zstd", 1) + // If suffix contains "gzip", replace it; otherwise append encoding indicator + if strings.Contains(suffix, "gzip") { + suffix = strings.Replace(suffix, "gzip", "zstd", 1) + } else if !strings.Contains(suffix, "zstd") { + suffix = suffix + "-zstd" + } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdgzhttp/compress.gogzhttp/writer/gzkp/gzkp.gogzhttp/writer/interface.gogzhttp/writer/zstdkp/zstdkp.go
✅ Files skipped from review due to trivial changes (1)
- gzhttp/writer/gzkp/gzkp.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gzhttp/writer/zstdkp/zstdkp.go
🧰 Additional context used
🧬 Code graph analysis (1)
gzhttp/writer/interface.go (4)
gzhttp/writer/gzkp/gzkp.go (1)
Levels(78-80)gzhttp/writer/zstdkp/zstdkp.go (1)
Levels(74-76)gzhttp/writer/gzstd/stdlib.go (1)
Levels(79-81)zstd/internal/xxhash/xxhash.go (1)
New(39-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (1.23.x, windows-latest)
- GitHub Check: build (1.25.x, macos-latest)
- GitHub Check: build (1.25.x, windows-latest)
- GitHub Check: build (1.23.x, ubuntu-latest)
- GitHub Check: build (1.24.x, windows-latest)
- GitHub Check: build (1.23.x, macos-latest)
- GitHub Check: build (1.25.x, ubuntu-latest)
- GitHub Check: build (1.24.x, macos-latest)
- GitHub Check: build-special
- GitHub Check: build (1.24.x, ubuntu-latest)
- GitHub Check: fuzz-other (nounsafe)
- GitHub Check: fuzz-s2 (nounsafe)
- GitHub Check: fuzz-other ("noasm,nounsafe")
- GitHub Check: fuzz-s2 ("noasm,nounsafe")
- GitHub Check: fuzz-zstd (nounsafe)
- GitHub Check: fuzz-zstd ("noasm,nounsafe")
🔇 Additional comments (13)
README.md (1)
10-10: LGTM!Documentation accurately reflects the new dual compression support (gzip and zstd).
gzhttp/writer/interface.go (1)
46-61: LGTM!The
ZstdWriterandZstdWriterFactoryinterfaces follow the same pattern as the existing gzip equivalents, providing a clean and consistent API for pluggable zstd implementations. The omission of aZstdWriterExtequivalent is correct since zstd uses skippable frames (not headers) for jitter.gzhttp/compress.go (11)
86-93: LGTM!The encoding enum is well-designed with
encodingNoneas the zero value, ensuring safe defaults.
105-111: LGTM!Zstd fields in
GzipResponseWriterare well-structured, mirroring the gzip pattern and properly supporting jitter via skippable frames.
137-144: LGTM!The Write method correctly routes to the appropriate compression writer (gzip or zstd) based on which is initialized.
469-484: LGTM - RFC 8878 compliant skippable frame implementation.The skippable frame format is correct per RFC 8878 Section 3.1.2. The magic number
0x184D2A50is within the valid range (0x184D2A5X). The jitter size is validated inconfig.validate()(line 731-732) to be less thanmath.MaxUint16, so theuint32cast at line 478 is safe.
706-744: LGTM!Validation logic is comprehensive: it validates compression levels only when the respective encoding is enabled, ensures at least one encoding is active, and correctly limits jitter header validation to gzip (since zstd uses skippable frames instead).
994-1040: LGTM!The
selectEncodingfunction correctly implements RFC 2616 content negotiation with proper handling of edge cases: HEAD requests, missing headers, single encoding availability, and qvalue comparison with configurable preference tiebreaker.
1042-1060: LGTM!Clean implementation that correctly parses qvalues using the existing
parseCodinghelper, returning 0 when the encoding is not present (distinct from the default qvalue of 1.0 when present without explicit q=).
790-830: LGTM!New configuration options follow the existing functional options pattern with clear documentation. Default settings (both encodings enabled, zstd preferred, fastest compression level) are sensible for broad compatibility as stated in PR objectives.
486-538: LGTM!The
Flushmethod correctly handles both compression encodings, flushing the appropriate writer when active.
581-591: LGTM!Default configuration aligns with PR objectives: both encodings enabled, zstd preferred, conservative
SpeedFastestlevel for compatibility.
1-27: LGTM!Comprehensive package documentation clearly explains dual compression support, content negotiation behavior, and usage patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @gzhttp/compress.go:
- Around line 531-534: The call to w.startCompression(nil) inside Flush ignores
its returned error; update the Flush implementation to capture the error from
startCompression, and if non-nil either log the error (using the existing logger
on w or fmt.Errorf if no logger exists) and then call w.startPlain(), or
otherwise fall back to startPlain() when startCompression fails; ensure you
still satisfy the http.Flusher behavior by handling the error path in the same
Flush method.
🧹 Nitpick comments (1)
gzhttp/compress.go (1)
1051-1069: Consider unifying withparseEncodingGzip.This function is nearly identical to
parseEncodingGzip(lines 1093-1112), with the only difference being the parameterized encoding name. Consider consolidating them to reduce code duplication.Suggested refactor
The existing
parseEncodingGzipcould be replaced with calls toparseEncodingQValue:// parseEncodingGzip returns the qvalue of gzip compression. func parseEncodingGzip(s string) float64 { - s = strings.TrimSpace(s) - - for len(s) > 0 { - stop := strings.IndexByte(s, ',') - if stop < 0 { - stop = len(s) - } - coding, qvalue, _ := parseCoding(s[:stop]) - - if coding == "gzip" { - return qvalue - } - if stop == len(s) { - break - } - s = s[stop+1:] - } - return 0 + return parseEncodingQValue(s, "gzip") }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gzhttp/compress.gogzhttp/compress_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gzhttp/compress_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
gzhttp/compress.go (4)
gzhttp/writer/interface.go (4)
ZstdWriterFactory(54-61)ZstdWriter(47-51)Header(28-34)GzipWriterExt(20-25)gzhttp/writer/gzkp/gzkp.go (2)
Levels(78-80)NewWriter(59-67)gzhttp/writer/zstdkp/zstdkp.go (2)
Levels(74-76)NewWriter(63-71)gzhttp/writer/gzstd/stdlib.go (2)
Levels(79-81)NewWriter(60-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (1.25.x, macos-latest)
- GitHub Check: build (1.24.x, ubuntu-latest)
- GitHub Check: build (1.24.x, windows-latest)
- GitHub Check: build (1.23.x, macos-latest)
- GitHub Check: build (1.23.x, ubuntu-latest)
- GitHub Check: build (1.25.x, ubuntu-latest)
- GitHub Check: fuzz-zstd ("noasm,nounsafe")
- GitHub Check: fuzz-other ("noasm,nounsafe")
- GitHub Check: fuzz-s2 ("noasm,nounsafe")
- GitHub Check: build-special
- GitHub Check: fuzz-other (nounsafe)
🔇 Additional comments (10)
gzhttp/compress.go (10)
1-52: LGTM!The package documentation clearly explains the dual-encoding support and content negotiation behavior. Imports are appropriate for the new zstd functionality.
86-126: LGTM!The encoding enum and struct extensions are well-designed. The zstd fields appropriately mirror their gzip counterparts, and the
zstdJitterfield for skippable frame support is a clean approach.
137-220: LGTM!The Write method correctly routes data to either the gzip or zstd writer based on which is active. The ordering of checks is consistent throughout.
228-270: LGTM!The
startCompressionmethod handles encoding-specific header setup correctly. The ETag suffix transformation logic properly differentiates between gzip and zstd responses to avoid cache conflicts.
478-493: LGTM!The zstd skippable frame implementation correctly follows RFC 8878 Section 3.1.2. The magic number
0x184D2A50is within the valid range, and the frame format (magic + size + data) is correctly structured. Writing the skippable frame afterzw.Close()ensures the main zstd frame is complete before appending jitter.
580-661: LGTM!The wrapper initialization correctly sets conservative zstd defaults (SpeedFastest level, both encodings enabled, zstd preferred). The
GzipResponseWriteris properly initialized with all encoding-related fields.
715-752: LGTM!The validation logic is thorough. It correctly validates compression levels conditionally based on which encodings are enabled, ensures at least one encoding is active, and only validates gzip jitter support when gzip is enabled (zstd jitter uses skippable frames that don't require special writer support).
799-839: LGTM!The new option functions follow the established pattern and provide clear documentation. The
EnableGzipoption enables zstd-only configurations, which is a nice flexibility.
1003-1049: LGTM!The
selectEncodingfunction correctly implements RFC-compliant content negotiation. It properly handles the HEAD request edge case, respects qvalue comparisons, and uses the configurable preference when qvalues are equal.
424-476: LGTM!The
Closemethod correctly handles both writer types, properly closes and nils them, and writes the zstd skippable frame only when jitter data is present. The early return on zstd close error prevents writing a skippable frame to a failed stream.
The new version enabled both gzip and zstd compression support in gzhttp, with auto-negotiation and preference for zstd. See: klauspost/compress#1121 Also added a tests to check zstd compression and preference.
The new version enabled both gzip and zstd compression support in gzhttp, with auto-negotiation and preference for zstd. See: klauspost/compress#1121 Also added tests to check zstd compression and preference.
The new version enables both gzip and zstd compression support in gzhttp, with auto-negotiation and preference for zstd. See: klauspost/compress#1121 Also added tests to check zstd compression and preference.
Both gzip and zstd compression are now enabled by default. When a client supports both, zstd is preferred due to its better compression ratio and speed.
Zstd compression is enabled by default alongside gzip. When the client supports both, zstd is preferred because it typically offers better compression ratios and faster decompression.
The server uses
Accept-Encodingheader negotiation to select the best encoding:gzip→ response is gzip compressedzstd→ response is zstd compressedgzip;q=1.0, zstd;q=0.5) → higher qvalue winsDefault zstd settings are conservative for broad compatibility:
SpeedFastest(1) - maximum speedSummary by CodeRabbit
New Features
Security
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.