KTOR-7075: Zstd compression support #4936
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the """ WalkthroughZstandard (zstd) compression support was integrated into the codebase. This involved updating dependency management to include the zstd-jni library, introducing ZstdEncoder implementations for all platforms, adding zstd encoding/decoding utilities for JVM, extending the compression plugin configuration, and expanding the test suite to cover zstd-specific scenarios. Changes
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
| @@ -221,7 +249,6 @@ class CompressionTest { | |||
| header(HttpHeaders.AcceptEncoding, "*") | |||
| } | |||
| assertEquals(HttpStatusCode.Found, response.status) | |||
| assertEquals(textToCompress, response.bodyAsText()) | |||
There was a problem hiding this comment.
Deleted this line because previously 'Identity' compression algorithm was somehow chosen by default, so basically no compression was made. After I added support for zstd algorithm - it started to be selected, so response body is encoded with zstd now. I didn't find any logic that restricts compression on non 200 status code, so decided to remove this line. Please, if i'm wrong, point me to the place in code where I should look for that logic. Thanks!
There was a problem hiding this comment.
Kinda weird behaviour - I think because identity was added last in the default config, it was being chosen in cases where other applicable compression methods are of the same priority. To avoid changing the behaviour here, I'd just move zstd() in Config.default() up one line. I feel like the default encoder really ought to be gzip since it's the most commonly supported but we ought to avoid changing the default here in the wildcard case.
There was a problem hiding this comment.
Did that, but it didn't help. I tried debugging the test and discovered that the issue is in ContentEncoding.Context.encode, which uses a comparator that compares encoders first by quality and then by priority. However, this test doesn't define any quality parameter, so all encoders are considered equal except for the deflate encoder, whose priority is manually reduced. As a result, after sorting, Zstd becomes first and Identity second (I assume this is due to the map’s hashing algorithm placing Zstd before Identity). That's why the response is compressed with Zstd.
But anyway, I don’t fully understand the purpose of check I've deleted. It sets "Accept-Encoding" to "*", which means we’re allowed to encode the response using any encoder. Since most encoders have the same priority, the encoding algorithm is chosen based on quality. But in this case, there's no quality specifier, so the encoder is effectively selected by the hashing order of the map where the encoders are stored. Am I missing something?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
ktor-utils/build.gradle.kts (1)
26-32:⚠️ Potential issue
zstd-jnimust be a JVM-only dependency – build will break for JS/Native targets.
zstd-jniships a pure-JVM jar with native libraries inMETA-INF/lib/….
Adding it tocommonMainleaks a JVM-exclusive artefact into every platform compilation and Gradle will fail to resolve it for JS and native targets.Apply the minimal fix below:
-sourceSets { - commonMain.dependencies { - api(projects.ktorIo) - api(libs.kotlinx.serialization.core) - api(libs.zstd.jni) - } +sourceSets { + commonMain.dependencies { + api(projects.ktorIo) + api(libs.kotlinx.serialization.core) + } + jvmMain.dependencies { + api(libs.zstd.jni) + }
🧹 Nitpick comments (2)
ktor-utils/jsAndWasmShared/src/io/ktor/util/ContentEncodersJs.kt (1)
27-34: Consistent stub, but update KDoc.The surrounding KDocs still reference gzip/deflate. Consider adding a short note that zstd is a no-op on JS/WASM for now to avoid confusion.
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (1)
29-35: Consider avoiding GlobalScopeUsing
GlobalScopeis generally discouraged as it can lead to resource leaks and makes testing difficult. Consider accepting aCoroutineScopeparameter or using a more appropriate scope.The current implementation works but could be improved by:
- Accepting a
CoroutineScopeparameter instead of usingGlobalScope- Adding proper exception handling for Zstd stream operations
- Considering cancellation scenarios
Would you like me to suggest an alternative implementation that addresses these concerns?
Also applies to: 42-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
gradle/libs.versions.toml(2 hunks)ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Config.kt(2 hunks)ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/CompressionTest.kt(12 hunks)ktor-utils/build.gradle.kts(1 hunks)ktor-utils/common/src/io/ktor/util/ContentEncoder.kt(1 hunks)ktor-utils/jsAndWasmShared/src/io/ktor/util/ContentEncodersJs.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/ContentEncodersJvm.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/EncodersJvm.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt(1 hunks)ktor-utils/posix/src/io/ktor/util/ContentEncodersNative.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (1)
ktor-io/jvm/src/io/ktor/utils/io/jvm/javaio/Blocking.kt (1)
toInputStream(20-49)
🔇 Additional comments (7)
gradle/libs.versions.toml (1)
80-81:❓ Verification inconclusive
Double-check artefact coverage and consider bumping to latest patch.
zstd-jni1.5.7-3 is fine but already a couple of patch levels behind (¹.⁵.⁷-5 / ¹.⁵.⁸ are published).
Newer builds add native artefacts for Apple-Silicon and fix a seg-fault on musl-based Alpine. Upgrading now avoids follow-up churn in downstream images.Also applies to: 240-241
🏁 Script executed:
#!/usr/bin/env bash # Verify the newest version and list all classifiers published on Maven Central. curl -s https://search.maven.org/solrsearch/select?q=g:%22com.github.luben%22+AND+a:%22zstd-jni%22\&rows=20\&wt=json \ | jq '.response.docs[] | {v: .v, hasArm64: (.ec[] | contains("osx-aarch_64"))) }'Length of output: 488
Let’s re-run the Maven Central query with corrected
jqsyntax to confirm that both1.5.7-5and1.5.8exist and include the Apple-Silicon classifier:
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail # Fetch zstd-jni versions and check for osx-aarch_64 binaries curl -s 'https://search.maven.org/solrsearch/select?q=g:%22com.github.luben%22+AND+a:%22zstd-jni%22&rows=100&wt=json' \ | jq '.response.docs[] | select(.v == "1.5.7-5" or .v == "1.5.8") | {version: .v, hasOsxArm64: (.ec | index("osx-aarch_64") != null)}'Length of output: 288
To confirm exactly which patch releases are available on Maven Central, let’s first list all published versions:
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail # List all zstd-jni versions published on Maven Central curl -s 'https://search.maven.org/solrsearch/select?q=g:%22com.github.luben%22+AND+a:%22zstd-jni%22&rows=200&wt=json' \ | jq -r '.response.docs[].v' \ | sort -V | uniqLength of output: 190
To reliably list all published zstd-jni versions and check for Apple-Silicon binaries, let’s use a simpler query with proper URL encoding:
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail curl -s "https://search.maven.org/solrsearch/select?q=g:com.github.luben%20AND%20a:zstd-jni&rows=200&wt=json" \ | jq '.response.docs[] | {version: .v, hasOsxArm64: (.ec | map(select(test("osx-aarch_64"))) | length > 0)}'Length of output: 355
Let’s directly query the Maven “maven2” repository HTML listings for each patch release to see which include the
osx-aarch_64classifier:
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail versions=(1.5.7-3 1.5.7-4 1.5.7-5 1.5.8) for v in "${versions[@]}"; do echo -n "$v: " curl -s "https://repo1.maven.org/maven2/com/github/luben/zstd-jni/$v/" \ | grep -q 'osx-aarch_64' \ && echo "includes Apple-Silicon binaries" \ || echo "no Apple-Silicon binaries" doneLength of output: 932
Let’s pull the actual JAR listings for each patch to see which classifiers are published:
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail versions=(1.5.7-3 1.5.7-4 1.5.7-5 1.5.8) for v in "${versions[@]}"; do echo "Version $v:" curl -s "https://repo1.maven.org/maven2/com/github/luben/zstd-jni/$v/" \ | grep -Eo 'href="[^"]+\.jar"' \ | sed -E 's/.*href="([^"]+)".*/\1/' \ | sort echo doneLength of output: 1195
Double-check artifact coverage and consider bumping to the latest patch (1.5.8)
While 1.5.7-3 already ships the
darwin_aarch64(Apple-Silicon) binaries, 1.5.7-5 and 1.5.8 introduce the Alpine (musl) seg-fault fix and any other upstream patch-level improvements. Upgrading now will avoid follow-up churn in downstream images.• gradle/libs.versions.toml (lines 80–81 and 240–241)
- zstd-jni = "1.5.7-3" + zstd-jni = "1.5.8"• Please verify that version 1.5.8 (or at least 1.5.7-5) is published on Maven Central and includes the musl-based Alpine fix.
ktor-utils/posix/src/io/ktor/util/ContentEncodersNative.kt (1)
25-27: LGTM – mirrors existing gzip/deflate stubs for POSIX targets.Until a native zstd binding is introduced, delegating to
Identitykeeps behaviour consistent with the other encoders on this platform.ktor-utils/jvm/src/io/ktor/util/ContentEncodersJvm.kt (1)
25-32: Nice – JVM encoder correctly delegates to the streamingZstdimplementation.Please ensure that
Zstdcloses its wrapped channels to avoid file-descriptor leaks (I assume this is handled inZstdEncoding.kt, just flagging it for a quick double-check).ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Config.kt (1)
104-104: LGTM!The addition of
zstd()to the default compression configuration is consistent with the pattern of including all supported encoders.ktor-utils/common/src/io/ktor/util/ContentEncoder.kt (1)
79-102: LGTM!The
ZstdEncoderdeclaration follows the established pattern for content encoders and is properly documented.ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/CompressionTest.kt (1)
7-7: Comprehensive test coverage for Zstd compressionThe test additions provide excellent coverage for the new Zstd compression feature, including:
- Default compression selection
- Wildcard encoding acceptance
- Minimum size thresholds
- Quality-based priority handling
- Layered encodings
- Request decompression
The tests follow established patterns and ensure the Zstd integration works correctly.
Also applies to: 103-113, 174-186, 302-323, 415-429, 533-533, 714-719, 746-751, 783-788, 790-791, 951-954, 980-980
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (1)
2-2: Verify copyright yearThe copyright year is set to 2025, which seems incorrect.
Please verify if this should be 2024 instead, as indicated in the PR submission date.
...-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Config.kt
Outdated
Show resolved
Hide resolved
tre3p
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (2)
20-23: Default dispatcher should be IO-oriented, notUnconfined(De)compression happens on blocking
Zstd*Streams. UsingDispatchers.Unconfinedrisks running blocking code on caller threads.
Recommend defaulting toDispatchers.IOon JVM.
91-101: Account forarrayOffset()to prevent incorrect offsets
buf.array()may be backed by a non-zeroarrayOffset().
Pass the correct offset when writing:- zstdStream.write(buf.array(), buf.position(), buf.remaining()) + zstdStream.write( + buf.array(), + buf.arrayOffset() + buf.position(), + buf.remaining() + )This makes the encoder resilient to alternative buffer pools that return sliced buffers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-utils/jvm/src/io/ktor/util/EncodersJvm.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-utils/jvm/src/io/ktor/util/EncodersJvm.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (1)
ktor-io/jvm/src/io/ktor/utils/io/jvm/javaio/Blocking.kt (1)
toInputStream(20-49)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-compression/api/ktor-server-compression.api (1)
75-76: Make sure the new public API is documented and reflected in user-facing docs
zstdand its default overload are now part of the stableConfigKtsurface.
Unlikegzip,deflate, andidentity, this brand-new encoder lacks any KDoc in the source set and isn’t yet mentioned in the official documentation website. Publishing an undocumented public function can lock the signature in stone while leaving users guessing about behaviour (e.g. default level, support matrix, minimum size defaults, etc.).Action items
- Add KDoc to
Config.ktexplaining the encoder’s parameters, defaults, and platform support.- Update the “Compression” section of ktor.io to list
zstdalongside existing encoders.- If the API is still experimental, guard it with
@OptIn(ExperimentalApi::class)instead of exposing it as stable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-server/ktor-server-plugins/ktor-server-compression/api/ktor-server-compression.api(1 hunks)ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ktor-utils/api/ktor-utils.klib.api (1)
896-903: Public API surface grows – double-check cross-platform availability & completeness
ZstdEncoderis now a common, public singleton that implements bothContentEncoderandEncoder.
Before the merge, please ensure:
- All targets declared in the header (
js,wasmJs, every Native target, etc.) have actual implementations that really work or, at minimum, throw a clearly-documentedNotImplementedError. Accidentally shipping stubs that silently fall back to identity encoding would be misleading.- The
nameproperty returns the canonical lower-case value"zstd"to stay consistent withgzip,deflate, etc. (header matching is case-insensitive but consistency avoids surprises).- Consider overriding
predictCompressedLength.IdentityEncoderoverrides it whileDeflate/GZipdon’t. Decide intentionally – if an estimation formula isn’t feasible, explicitly document that it returnsnull.- Add conformance tests similar to the existing gzip/deflate suites for every supported engine to guard against regressions.
ktor-utils/api/ktor-utils.api (2)
194-198: ExposegetZstd()consistently across all targets
EncodersJvmKtnow publishesgetZstd(), but the API file only lists the JVM variant.
Please verify that:
expect fun getZstd(): Encoderis declared in the common source-set (mirroringgetGZip/getDeflate).- Every platform has an
actualimplementation so that a multiplatform consumer can callEncoders.getZstd()withoutexpect/actualgaps.Otherwise the public‐API is JVM-only, which breaks parity with the other encoders and forces platform-specific work-arounds.
507-515: Minor API polish forZstdEncodingKthelpers
- The object-pool parameter is mandatory while
DeflaterKt& friends default toKtorDefaultPool. Providing an overload without the pool argument would keep the ergonomics symmetric.- The overload set is missing
encoded(ByteWriteChannel, CoroutineContext)/decoded(ByteWriteChannel, …)counterparts; the gzip/deflate helpers expose both read- and write-side shortcuts.Aligning these signatures with the existing compression helpers avoids surprise for plugin authors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (1)
28-35: GlobalScope still used – coroutine lifetime is VM-wideThe functions still launch writers/readers in
GlobalScope, so the coroutines out-live the caller and may leak.
Expose aCoroutineScopeparameter (or usecoroutineScope { … }) and launch inside that scope instead.-public fun ByteReadChannel.encoded( - pool: ObjectPool<ByteBuffer> = KtorDefaultPool, - coroutineContext: CoroutineContext = Dispatchers.Unconfined -): ByteReadChannel = GlobalScope.writer(coroutineContext, autoFlush = true) { +public fun ByteReadChannel.encoded( + pool: ObjectPool<ByteBuffer> = KtorDefaultPool, + scope: CoroutineScope, + coroutineContext: CoroutineContext = Dispatchers.Unconfined +): ByteReadChannel = scope.writer(coroutineContext, autoFlush = true) {Apply the same pattern to the other two helpers.
Also applies to: 41-48, 54-60
🧹 Nitpick comments (3)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (3)
75-81: Potential decompression overflow
outputBuf.capacity()is passed as the maximum output size. A zstd frame can easily expand beyond the input buffer size; if the frame is larger than the borrowed buffer this call fails withZstdException.
Consider either:
- Allocating
outputBufwithZstdDecompressCtx.getDecompressedSize()when the frame size is known, or- Switching to the streaming API (
ZstdInputStreamNoFinalizer) which handles chunk sizing internally.
93-95: Direct use ofByteBuffer.array()assumes heap buffers
KtorDefaultPoolmay return direct buffers in future;ByteBuffer.array()would then throwUnsupportedOperationException.
UseByteBuffer.hasArray()guard or allocate a temporary heap array.val inArr = if (inputBuf.hasArray()) inputBuf.array() else ByteArray(bytesRead).also { inputBuf.get(it) }Same issue for the
outputBufpath.Also applies to: 103-108
94-94: Unneeded semicolonIdiomatic Kotlin omits the trailing semicolon.
-val outputBuf = pool.borrow(); +val outputBuf = pool.borrow()
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (1)
98-98: Remove unnecessary semicolon.Minor style improvement for consistency with Kotlin conventions.
- val outputBuf = pool.borrow(); + val outputBuf = pool.borrow()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
gradle/libs.versions.toml(2 hunks)ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Config.kt(4 hunks)ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/CompressionTest.kt(13 hunks)ktor-utils/common/src/io/ktor/util/ContentEncoder.kt(1 hunks)ktor-utils/jsAndWasmShared/src/io/ktor/util/ContentEncodersJs.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/ContentEncodersJvm.kt(2 hunks)ktor-utils/jvm/src/io/ktor/util/EncodersJvm.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt(1 hunks)ktor-utils/posix/src/io/ktor/util/ContentEncodersNative.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ktor-utils/posix/src/io/ktor/util/ContentEncodersNative.kt
- ktor-utils/jvm/src/io/ktor/util/EncodersJvm.kt
- gradle/libs.versions.toml
- ktor-utils/common/src/io/ktor/util/ContentEncoder.kt
- ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Config.kt
- ktor-utils/jsAndWasmShared/src/io/ktor/util/ContentEncodersJs.kt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{kt,kts}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.{kt,kts,json,yaml,yml}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/test/**/*.kt
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p removed ZstdInputStream/ZstdOutputStream from their ZstdEncoder implementation to address blocking I/O concerns in coroutine contexts.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p is planning to implement a configurable factory pattern for ZstdEncoder instead of using a singleton INSTANCE, to allow customization of compression levels and dictionaries.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
ktor-utils/jvm/src/io/ktor/util/ContentEncodersJvm.kt (3)
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p removed ZstdInputStream/ZstdOutputStream from their ZstdEncoder implementation to address blocking I/O concerns in coroutine contexts.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p is planning to implement a configurable factory pattern for ZstdEncoder instead of using a singleton INSTANCE, to allow customization of compression levels and dictionaries.
ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/CompressionTest.kt (12)
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p removed ZstdInputStream/ZstdOutputStream from their ZstdEncoder implementation to address blocking I/O concerns in coroutine contexts.
Learnt from: rururux
PR: ktorio/ktor#4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Learnt from: rururux
PR: ktorio/ktor#4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p is planning to implement a configurable factory pattern for ZstdEncoder instead of using a singleton INSTANCE, to allow customization of compression levels and dictionaries.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/test/**/*.kt : Follow test naming pattern: DescribeWhatIsBeingTested
Learnt from: bjhham
PR: ktorio/ktor#4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Error handling follows Kotlin conventions with specific Ktor exceptions
Learnt from: rururux
PR: ktorio/ktor#4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Learnt from: rururux
PR: ktorio/ktor#4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Use star imports for io.ktor.* packages (configured in .editorconfig)
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (16)
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p removed ZstdInputStream/ZstdOutputStream from their ZstdEncoder implementation to address blocking I/O concerns in coroutine contexts.
Learnt from: tre3p
PR: ktorio/ktor#4936
File: ktor-utils/api/ktor-utils.api:498-506
Timestamp: 2025-06-16T17:13:12.657Z
Learning: The user tre3p is planning to implement a configurable factory pattern for ZstdEncoder instead of using a singleton INSTANCE, to allow customization of compression levels and dictionaries.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Error handling follows Kotlin conventions with specific Ktor exceptions
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Use star imports for io.ktor.* packages (configured in .editorconfig)
Learnt from: bjhham
PR: ktorio/ktor#4786
File: ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1ApplicationResponse.kt:114-114
Timestamp: 2025-04-08T10:04:54.762Z
Learning: When reviewing PRs, ensure that fixes implemented by the PR author are not labeled as "Potential issues" but rather acknowledged as proper solutions. The heading should match the content of the comment.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Prefix commit messages with KTOR-{NUM} (YouTrack issue)
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Prefix commit messages with KTOR-{NUM} (YouTrack issue), and write them in present tense, imperative mood.
Learnt from: bjhham
PR: ktorio/ktor#4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
Learnt from: osipxd
PR: ktorio/ktor#4970
File: build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts:67-72
Timestamp: 2025-07-01T10:54:53.751Z
Learning: In build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts, the KT-70915 and KT-77609 workarounds that limit parallelism for KotlinNativeLink and CInteropCommonizerTask should remain unconditional (not gated behind CI flag) as these limits are beneficial for both local and CI builds.
Learnt from: bjhham
PR: ktorio/ktor#4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T08:41:41.935Z
Learning: Applies to **/*.{kt,kts} : Follow Kotlin official style guide
Learnt from: rururux
PR: ktorio/ktor#4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Learnt from: rururux
PR: ktorio/ktor#4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Learnt from: bjhham
PR: ktorio/ktor#4916
File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151
Timestamp: 2025-06-09T07:08:35.085Z
Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
🔇 Additional comments (10)
ktor-server/ktor-server-tests/jvm/test/io/ktor/server/plugins/CompressionTest.kt (7)
103-113: Excellent test coverage for zstd default compression selection.The test properly validates that zstd is selected when included in the Accept-Encoding header with other encoders, ensuring correct priority handling.
173-186: Good test coverage for wildcard Accept-Encoding with zstd.This test validates that zstd compression works correctly when the client accepts any encoding (*), which is important for fallback scenarios.
301-323: Comprehensive minimum size testing for zstd.The test properly validates the minimum size threshold behavior specific to zstd compression, ensuring consistency with other encoders.
366-381: Excellent validation of configurable compression levels.This test verifies that custom compression levels are properly applied to the zstd encoder, which is crucial for performance tuning.
730-745: Thorough basic encode/decode test implementation.The test validates both default and custom compression levels, ensuring the core compression/decompression functionality works correctly at different quality settings.
772-777: Complete zstd decoding test coverage.The tests validate both request decompression and multi-layered encoding scenarios, ensuring robust support for zstd in request/response processing.
Also applies to: 809-813
1006-1006: Well-implemented readZstd helper method.The helper method follows the established pattern of other compression readers (readGzip, readDeflate) and correctly uses ZstdInputStream for decompression validation.
ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt (3)
28-35: Consistent use of GlobalScope pattern for byte channel encoding.The implementation correctly follows the established Ktor pattern of using
GlobalScope.writerfor byte channel transformations, maintaining consistency with other encoders in the codebase.
64-90: Excellent resource management in decodeTo function.The implementation properly manages resources with try/finally blocks, ensuring ZstdDecompressCtx is closed and buffers are recycled even in error scenarios. The buffer clearing and error handling logic is correct.
92-119: Robust compression implementation with proper resource cleanup.The encodeTo function demonstrates good practices with proper resource management, buffer pooling, and compression context handling. The compression level configuration is correctly applied.
|
@bjhham Hey! Made some changes according to your comments, thanks for them! Please, take a look at this changes and also at my comment about existing test I've changed. Thanks! |
| @@ -27,6 +27,7 @@ kotlin { | |||
| commonMain.dependencies { | |||
| api(projects.ktorIo) | |||
| api(libs.kotlinx.serialization.core) | |||
| api(libs.zstd.jni) | |||
There was a problem hiding this comment.
Could you change this to implementation()
There was a problem hiding this comment.
Is it necessary? I'm using ZstdInputStream in ktor-server-tests
…/ decoding process to avoid blocking
tre3p
force-pushed
the
KTOR-7075
branch
2 times, most recently
from
13cd410 to
95a1b20
Compare
Subsystem
ktor-utils module, Compression plugin
Motivation
This PR solves KTOR-7075 ticket by adding support for zstd compression algorithm to Compression plugin.
Solution
I've used zstd-jni library which provides JNI binding to zstd compression algorithm implementation.