Migrate FileCacheStorage to common using kotlinx.io #4940
Conversation
WalkthroughThis update migrates Ktor's HTTP cache file storage and related tests from JVM-specific file APIs to multiplatform file system abstractions using Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt (1)
145-154: Duplicate directory helpers – consider extractingSame helper exists in
FileStorageTest. Moving it to a shared test-util keeps
tests DRY.
🧹 Nitpick comments (11)
ktor-io/common/src/io/ktor/utils/io/core/Reading.kt (1)
22-24: Drop the misleading @Suppress and expose a sane default dispatcher
contextis actually used, so the@Suppress("UNUSED_PARAMETER")annotation is noise.
In addition, callers nearly always want an I/O-optimised dispatcher; providingDispatchers.IOas a default makes the API much more convenient.-@Suppress("UNUSED_PARAMETER") -public fun RawSource.toByteReadChannel(context: CoroutineContext): ByteReadChannel = - RawSourceChannel(this, context) +public fun RawSource.toByteReadChannel( + context: CoroutineContext = Dispatchers.IO, +): ByteReadChannel = RawSourceChannel(this, context)ktor-utils/jsAndWasmShared/src/io/ktor/util/CryptoJs.kt (1)
67-72: Fix copy-paste artefacts in KDoc linksThe KDoc block still references
sha1; update it so generated documentation and “report a problem” links point to the right symbol.- * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha1) + * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha256)ktor-utils/jvm/src/io/ktor/util/CryptoJvm.kt (1)
40-46: Synchronise KDoc with the newsha256helperSame documentation glitch as on JS: the link and description still mention
sha1.- * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha1) + * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha256)ktor-utils/posix/src/io/ktor/util/CryptoNative.kt (1)
35-39: Update KDoc reference for the new SHA-256 APIMirror the documentation fix applied to the other platforms.
- * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha1) + * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha256)ktor-utils/common/src/io/ktor/util/Crypto.kt (1)
80-85: Doc-block still referencessha1– update wording & linkCopy-pasted Javadoc left the “sha1” wording and fqname in the SHA-256 section.
This will trip documentation generators and mislead users skimming the source.- * Compute SHA-256 hash for the specified [bytes] - * - * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha1) + * Compute SHA-256 hash for the specified [bytes] + * + * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.util.sha256)ktor-client/ktor-client-core/common/test/FileStorageTest.kt (2)
18-20: ExtensiondeleteRecursivelyis file-local – qualify the call
SystemFileSystem.deleteRecursively(tempDirectory)relies on the extension
defined inside the companion object, which is not imported into the
receiver’s scope outside this class in other files.If you intend this helper to be used broadly, make it top-level in a shared
test util or call it through the companion:FileStorageTest.deleteRecursively(SystemFileSystem, tempDirectory)(or move it to a common test-util package).
96-112: Helper duplicated across two test classes
temporaryDirectoryPath()andFileSystem.deleteRecursively()appear in both
FileStorageTestandFileCacheTest. Extract them to a single
test/io/ktor/util/TestFileHelpers.ktto avoid divergence.ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/CachingCacheStorage.kt (1)
14-45: Potential race & memory-leak in in-memory mirror
- The
Set<CachedResponseData>instances stored inConcurrentMapare
ordinary (mutable) HashSets – not thread-safe. Two coroutines may mutate the
same set concurrently when delegate updates it.- Storing the whole delegate snapshot indefinitely can grow without bound and
defeats the purpose of a file-backed cache.Consider:
private val store = ConcurrentMap<Url, List<CachedResponseData>>() // immutable copy // after any mutation store[url] = delegate.findAll(url).toList() // fresh immutable snapshotand periodically pruning the map or exposing a
maxEntriesparameter.ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt (2)
25-32:install(HttpCache.Companion)is unusual API usageThe canonical form is
install(HttpCache). Using the companion object is a
by-product of Kotlin’s objects being both the type and the instance and is
unnecessary noise. Prefer the idiomatic call:config { install(HttpCache) { … } }
99-101: Use enum singleton directly
HttpStatusCode.Companion.OK→HttpStatusCode.OKfor succinctness.ktor-utils/common/src/io/ktor/util/HashFunction.kt (1)
422-423: Consider removing the customandextension functionThe custom
andextension function is unnecessary since Kotlin already provides bitwise operations forByte.toInt().You can simplify the code by directly using:
input[currentPosition].toInt() and 0xffThis would make the code more idiomatic and reduce the need for the companion object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/CachingCacheStorage.kt(1 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt(5 hunks)ktor-client/ktor-client-core/common/test/FileStorageTest.kt(6 hunks)ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/JvmFileCacheStorage.kt(1 hunks)ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt(3 hunks)ktor-io/common/src/io/ktor/utils/io/core/Reading.kt(1 hunks)ktor-io/common/src/io/ktor/utils/io/core/Writing.kt(1 hunks)ktor-io/jvm/src/io/ktor/utils/io/jvm/javaio/Reading.kt(0 hunks)ktor-io/jvm/src/io/ktor/utils/io/jvm/nio/Reading.kt(1 hunks)ktor-utils/common/src/io/ktor/util/Crypto.kt(1 hunks)ktor-utils/common/src/io/ktor/util/HashFunction.kt(1 hunks)ktor-utils/jsAndWasmShared/src/io/ktor/util/CryptoJs.kt(1 hunks)ktor-utils/jvm/src/io/ktor/util/CryptoJvm.kt(1 hunks)ktor-utils/posix/src/io/ktor/util/CryptoNative.kt(1 hunks)
💤 Files with no reviewable changes (1)
- ktor-io/jvm/src/io/ktor/utils/io/jvm/javaio/Reading.kt
🧰 Additional context used
🧠 Learnings (1)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt (2)
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.
🧬 Code Graph Analysis (4)
ktor-io/common/src/io/ktor/utils/io/core/Writing.kt (1)
ktor-io/common/src/io/ktor/utils/io/core/Reading.kt (1)
awaitContent(46-67)
ktor-client/ktor-client-core/common/test/FileStorageTest.kt (3)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt (2)
temporaryDirectoryPath(140-143)deleteRecursively(145-154)ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/JvmFileCacheStorage.kt (1)
FileStorage(28-33)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (1)
FileStorage(30-35)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt (5)
ktor-client/ktor-client-core/common/test/FileStorageTest.kt (2)
temporaryDirectoryPath(97-100)deleteRecursively(102-111)ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/JvmFileCacheStorage.kt (1)
FileStorage(28-33)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (1)
FileStorage(30-35)ktor-client/ktor-client-test-base/common/src/io/ktor/client/test/base/ClientLoader.kt (1)
clientTests(33-57)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt (2)
publicStorage(123-125)privateStorage(136-138)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (4)
ktor-utils/common/src/io/ktor/util/Crypto.kt (3)
hex(24-36)hex(43-53)sha256(85-85)ktor-utils/jsAndWasmShared/src/io/ktor/util/CryptoJs.kt (1)
sha256(72-72)ktor-utils/jvm/src/io/ktor/util/CryptoJvm.kt (1)
sha256(45-46)ktor-utils/posix/src/io/ktor/util/CryptoNative.kt (1)
sha256(39-39)
🔇 Additional comments (5)
ktor-io/jvm/src/io/ktor/utils/io/jvm/nio/Reading.kt (1)
8-8: Import path update looks correctSwitching from the JVM-specific package to the new common implementation is appropriate and keeps the JVM façade thin.
No action required.ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/JvmFileCacheStorage.kt (1)
20-34: LGTM! Well-structured JVM-specific implementationThe deprecation notice is clear and the implementation properly maintains backward compatibility while directing users to the new multiplatform version.
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (3)
31-35: LGTM! Successful migration to multiplatform APIsThe function signature has been properly updated to use
FileSystemandPathfrom kotlinx.io, enabling multiplatform support as intended by the PR objectives.
79-79: Excellent use of multiplatform crypto APIThe migration from JVM-specific
MessageDigestto the multiplatformsha256function aligns perfectly with the PR's goal of enabling cross-platform file cache functionality.
114-136: Clean migration to kotlinx.io file operationsThe file I/O operations have been successfully migrated from JVM-specific streams to multiplatform
SourceandSinkabstractions, maintaining the same functionality while enabling cross-platform support.
| override suspend fun awaitContent(min: Int): Boolean { | ||
| if (closedToken != null) return true | ||
|
|
There was a problem hiding this comment.
awaitContent short-circuits incorrectly on closed channels
Returning true when the channel is already closed but the buffer is empty violates the contract of awaitContent (it should return false when no more data can arrive).
This will trick callers into attempting a read that immediately hits EOF.
-if (closedToken != null) return true
+if (closedToken != null) return buffer.remaining >= min🤖 Prompt for AI Agents
In ktor-io/common/src/io/ktor/utils/io/core/Reading.kt around lines 46 to 48,
the awaitContent function incorrectly returns true when the channel is closed
but the buffer is empty, which violates its contract. Modify the logic so that
if the channel is closed and no more data is available, awaitContent returns
false instead of true, ensuring callers do not attempt reads that immediately
hit EOF.
There was a problem hiding this comment.
Agree, but this is already the implementation from the JVM side where it was taken, and I wanted to restrict my changes as much as possible
There was a problem hiding this comment.
@bishiboosh That's a completely reasonable approach for a migration PR - keeping changes minimal reduces risk and keeps the scope focused.
Would you like me to create a follow-up issue to track this awaitContent bug for a future fix? Since it's a pre-existing issue that affects the contract correctness, having it tracked would ensure it doesn't get overlooked.
✏️ Learnings added
Learnt from: bishiboosh
PR: ktorio/ktor#4940
File: ktor-io/common/src/io/ktor/utils/io/core/Reading.kt:46-48
Timestamp: 2025-06-17T12:18:26.319Z
Learning: User bishiboosh prefers to keep changes minimal during migration PRs, even when pre-existing issues are identified, to maintain focus on the migration scope and reduce risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Honestly I'll let the maintainers decide on that, but why not
There was a problem hiding this comment.
🧩 Analysis chain
I'll create the follow-up issue to track this bug for the maintainers.
@bishiboosh Issue created! The maintainers can now track this bug and decide when to address it in a future PR.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "awaitContent returns incorrect value when channel is closed with empty buffer" \
--body "## Description
The \`awaitContent\` function in \`RawSourceChannel\` incorrectly returns \`true\` when the channel is already closed but the buffer is empty. This violates the contract of \`awaitContent\`, which should return \`false\` when no more data can arrive.
## Current Behavior
When \`closedToken != null\`, the function immediately returns \`true\` regardless of buffer state.
## Expected Behavior
When the channel is closed and insufficient data is available in the buffer, \`awaitContent\` should return \`false\` to indicate that the minimum required data cannot be satisfied.
## Impact
This causes callers to attempt reads that immediately hit EOF, as they're misled into thinking sufficient data is available.
## Context
This issue was identified during migration PR #4940 but kept unchanged to minimize migration scope. The bug exists in both the original JVM implementation and the new common implementation.
## References
- PR: https://github.com/ktorio/ktor/pull/4940
- Discussion: https://github.com/ktorio/ktor/pull/4940#discussion_r2152115539
- Reporter: @bishiboosh" \
--assignee bishiboosh
Length of output: 1260
Perfect! Issue #4941 has been created successfully: #4941
The maintainers now have a proper tracking issue for the awaitContent bug with all the relevant context and references. Thanks for being open to creating it - it ensures this contract violation won't get lost in the shuffle.
🧬 Code Graph Analysis Results
Relevant snippet from ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerRequestScope.kt (lines 36-44)
public fun withContext(coroutineContext: CoroutineContext): ServerRequestScope =
ServerRequestScope(
this.coroutineContext + coroutineContext,
input,
output,
remoteAddress,
localAddress,
upgraded
)This function extends ServerRequestScope by adding additional CoroutineContext elements to the existing context, returning a new ServerRequestScope instance with the combined context and the same input, output, and address properties. This is relevant for understanding how coroutine contexts are composed and passed around, which relates to the use of CoroutineContext in RawSourceChannel in Reading.kt.
| @OptIn(InternalAPI::class, InternalIoApi::class) | ||
| public suspend fun ByteReadChannel.copyTo(out: RawSink, limit: Long = Long.MAX_VALUE): Long { | ||
| require(limit >= 0) { "Limit shouldn't be negative: $limit" } | ||
| var result = 0L | ||
| while (!isClosedForRead) { | ||
| if (readBuffer.exhausted()) awaitContent() | ||
| result += readBuffer.buffer.size | ||
| readBuffer.buffer.transferTo(out) | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
limit parameter is ignored and can cause unbounded copies
The implementation copies the whole channel until EOF regardless of the limit
argument. This violates the contract and may exhaust memory / disk when users
expect a hard cap.
It also:
- Adds the pre-transfer buffer size to
result, so the last increment happens
even if nothing is written whenawaitContent()hits EOF. - Never flushes the
RawSink. - Can overshoot
Long.MAX_VALUEon pathological streams.
@OptIn(InternalAPI::class, InternalIoApi::class)
public suspend fun ByteReadChannel.copyTo(
- out: RawSink, limit: Long = Long.MAX_VALUE
+ out: RawSink,
+ limit: Long = Long.MAX_VALUE
): Long {
require(limit >= 0) { "Limit shouldn't be negative: $limit" }
- var result = 0L
- while (!isClosedForRead) {
- if (readBuffer.exhausted()) awaitContent()
- result += readBuffer.buffer.size
- readBuffer.buffer.transferTo(out)
+ var copied = 0L
+ var remaining = limit
+
+ while (remaining > 0 && !isClosedForRead) {
+ if (readBuffer.exhausted()) awaitContent()
+
+ val toWrite = minOf(readBuffer.buffer.remaining, remaining)
+ if (toWrite == 0) break
+
+ out.writeFully(readBuffer.buffer, toWrite)
+ copied += toWrite
+ remaining -= toWrite
}
-
- return result
+ out.flush()
+ return copied
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ktor-io/common/src/io/ktor/utils/io/core/Writing.kt between lines 19 and 30,
the copyTo function ignores the limit parameter, causing unbounded copying and
incorrect result calculation. Fix this by tracking the total bytes copied and
stopping when the limit is reached, only adding to result the actual bytes
transferred each iteration. Also, flush the RawSink after copying completes to
ensure all data is written, and handle potential overflow by capping the result
at Long.MAX_VALUE.
There was a problem hiding this comment.
Same as above, this was in the original implementation, so I'm ok to modify it but that means I'll have to modify the JVM version too. A better idea would be to remove the limit parameter altogether.
There was a problem hiding this comment.
🧩 Analysis chain
@bishiboosh, good point about the original implementation. Let me check the current usage patterns and the JVM implementation to help evaluate the best approach.
🏁 Script executed:
#!/bin/bash
# Search for existing usages of copyTo function with ByteReadChannel
rg "copyTo\(" --type kt -A 2 -B 2
# Also search for the JVM implementation mentioned
rg "fun.*copyTo.*RawSink" --type kt -A 5
# Search for any imports or references to this specific copyTo function
rg "io\.ktor\.utils\.io\.core\.copyTo" --type kt
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Search for existing usages of copyTo function with correct file extension
rg "copyTo\(" --glob "*.kt" -A 2 -B 2
# Search for JVM implementation
rg "fun.*copyTo.*RawSink" --glob "*.kt" -A 5
# Search for ByteReadChannel.copyTo usages specifically
rg "ByteReadChannel\.copyTo" --glob "*.kt" -A 2 -B 2
# Look for existing copyTo implementations in jvm directories
fd -e kt . | xargs grep -l "copyTo" | grep jvm
Length of output: 29174
🏁 Script executed:
#!/bin/bash
# Check the JVM implementation to see if it has the same issue
cat "ktor-io/jvm/src/io/ktor/utils/io/jvm/javaio/Writing.kt"
Length of output: 977
@bishiboosh, you're absolutely right! Looking at the usage patterns, I can see that both implementations have identical issues with the limit parameter, and most of the explicit limit usage in the codebase appears to be with the ByteWriteChannel.copyTo overloads rather than the RawSink/OutputStream ones.
Removing the limit parameter would be the cleaner solution because:
- Consistency: Both implementations currently ignore it anyway
- Simplicity: Eliminates the broken functionality rather than maintaining it
- Usage patterns: The explicit limit usage I found seems to target other
copyTooverloads
Here's how the simplified implementation would look:
@OptIn(InternalAPI::class, InternalIoApi::class)
public suspend fun ByteReadChannel.copyTo(out: RawSink): Long {
var copied = 0L
while (!isClosedForRead) {
if (readBuffer.exhausted()) awaitContent()
val bytesToWrite = readBuffer.buffer.size
if (bytesToWrite == 0) break
readBuffer.buffer.transferTo(out)
copied += bytesToWrite
}
out.flush()
return copied
}This would also fix the other issues (incorrect result calculation and missing flush) while removing the unused parameter. You'd make the same change to the JVM OutputStream version.
What do you think about this approach?
🧬 Code Graph Analysis Results
Relevant snippet from ktor-io/common/src/io/ktor/utils/io/core/Reading.kt lines 46-67:
override suspend fun awaitContent(min: Int): Boolean {
if (closedToken != null) return true
withContext(coroutineContext) {
var result = 0L
while (buffer.remaining < min && result >= 0) {
result = try {
source.readAtMostTo(buffer, Long.MAX_VALUE)
} catch (cause: EOFException) {
-1L
}
}
if (result == -1L) {
source.close()
job.complete()
closedToken = CloseToken(null)
}
}
return buffer.remaining >= min
}This function is a suspending function that waits until at least min bytes are available in the buffer or the channel is closed. It reads from the source into the buffer, handling EOF by closing the source and marking the channel closed. It returns a Boolean indicating if the buffer has at least min bytes available.
No other snippets from the provided list are relevant to understanding or modifying the copyTo function in Writing.kt.
There was a problem hiding this comment.
I'll wait until a human maintainer can review and tell me if they wanna remove it or not but that may be the best idea
There was a problem hiding this comment.
@bishiboosh That's a very sensible approach! Getting maintainer input on API changes is definitely the right call, especially when it involves removing parameters that could affect backwards compatibility.
The analysis shows removing the limit parameter would be cleaner since it's not actually being used correctly in either implementation, but you're absolutely right to let the maintainers make that decision. They'll have the best perspective on whether there are any compatibility concerns or if there are planned uses for the parameter that we're not seeing.
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/FileCacheTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ktor-utils/common/src/io/ktor/util/HashFunction.kt (3)
216-224: Align parameter naming with the interface for consistency
HashFunction.updateuses the parameter namelength; here the override introducesbyteCount. While this compiles (names aren’t part of the JVM signature), the divergence is a small cognitive hurdle when reading implementations side-by-side. Renaming keeps mental mapping trivial.- input: ByteArray, - offset: Int, - byteCount: Int, + input: ByteArray, + offset: Int, + length: Int,Remember to adjust the internal references from
byteCount→length.
260-268: LocalByte.andhelper is convenient but narrowly scoped
input[cPos++] and 0xffrelies on theByte.andextension declared inside the companion object.
Because that helper isprivate, the call sites inside the class are fine, yet no other hashing utilities can reuse it. Moving it next to theleftRotatetop-level helper (and marking itinternal) would:
- Prevent accidental duplication in future algorithms.
- Remove the mild surprise of an infix extension “appearing” only inside this file.
-private inline infix fun Byte.and(other: Int): Int = toInt() and other +internal inline infix fun Byte.and(other: Int): Int = toInt() and other…and place it after
leftRotate.
335-346: Nit: drop the unused copy ofunprocessedLimit
var unprocessedLimit = this.unprocessedLimitis never read after creation. Deleting the local variable avoids dead code and a fleeting “why is this here?” moment.- var unprocessedLimit = this.unprocessedLimit + // `unprocessedLimit` is accessed directly from the field below; no local copy needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ktor-utils/common/src/io/ktor/util/HashFunction.kt(1 hunks)
🔇 Additional comments (2)
ktor-utils/common/src/io/ktor/util/HashFunction.kt (2)
72-82: Variable-shadowing issue resolved
currentPositionreplaces the previous shadowing ofpos, improving clarity and eliminating the risk of subtle bugs.
425-435: Static constant table looks correctThe SHA-256 round constants (
k) match the FIPS-180-4 specification. No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ktor-utils/common/src/io/ktor/util/HashFunction.kt (1)
21-23: Drop the redundant warning suppression or actually inline the helper
@Suppress("NOTHING_TO_INLINE")makes sense only when the function is markedinline; otherwise the suppression is a no-op and just clutters the file.
Either turn the helper into a real inline (preferred – the JVM/JS/Native inliner will eliminate the call completely) or remove the suppression.-@Suppress("NOTHING_TO_INLINE") // Syntactic sugar -private infix fun Byte.and(other: Int): Int = toInt() and other +@Suppress("NOTHING_TO_INLINE") // Syntactic sugar +private inline infix fun Byte.and(other: Int): Int = toInt() and other
Subsystem
Client file cache
Motivation
On-disk cache is only available on JVM right now. This PR intends to port it to a common implementation to be able to use it globally in KMP apps.
Solution
This PR moves the
FileStoragefrom its current JVM implementation to a common one usingkotlinx.io. Most of the code is just moved from its current implementation and adapted to use Source/Sink instead of InputStream/OutputStream.The only "new" code is a SHA-256 implementation for common, taken from the SHA-256 implementation in Okio.