Add clear API to HttpCache for clearing all cached data#5535
Open
fru1tworld wants to merge 3 commits intoktorio:mainfrom
Open
Add clear API to HttpCache for clearing all cached data#5535fru1tworld wants to merge 3 commits intoktorio:mainfrom
fru1tworld wants to merge 3 commits intoktorio:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds suspend-based cache purge APIs: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
ktor-client/ktor-client-core/api/ktor-client-core.api (1)
636-690: KDoc forCacheStorage.clear()should clarify the default behavior.The current KDoc ("Removes all entries from this cache storage") doesn't document that
clear()is a default no-op. Since all known implementations (InMemoryCacheStorage, CachingCacheStorage, FileCacheStorage, UnlimitedStorage) override it with actual cleanup logic, the KDoc should explicitly state that this is a default no-op implementation and that subclasses should override it to ensure proper resource cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-core/api/ktor-client-core.api` around lines 636 - 690, Update the KDoc for the interface method CacheStorage.clear() to state that the default implementation is a no-op and does not perform cleanup, and explicitly advise implementors to override clear() to perform any necessary resource or entry cleanup; reference the interface method CacheStorage.clear and the provided default implementation CacheStorage$DefaultImpls.clear, and mention common implementations (InMemoryCacheStorage, CachingCacheStorage, FileCacheStorage, UnlimitedStorage) as examples that should implement proper cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ktor-client/ktor-client-core/api/ktor-client-core.klib.api`:
- Line 413: clearAllCaches() currently only clears
publicStorageNew/privateStorageNew and misses legacy storages when useOldStorage
is true; to fix, add a clear() method to the legacy HttpCacheStorage
interface/implementation and update clearAllCaches() to detect useOldStorage (or
non-null publicStorage/privateStorage) and call
publicStorage.clear()/privateStorage.clear() for the legacy path, while still
clearing publicStorageNew/privateStorageNew for the modern path; also ensure
HttpCacheLegacy.kt code paths that use publicStorage/privateStorage rely on that
new clear() and mark the API change with deprecation notes for the old
properties.
In
`@ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt`:
- Around line 141-147: The clearAllCaches() implementation is only clearing
publicStorageNew/privateStorageNew and ignores legacy storages when
useOldStorage is true and also clears public before private (risking private not
being attempted on throw); update clearAllCaches() (and its KDoc) to either (A)
support legacy storages: if useOldStorage then clear privateStorage first then
publicStorage, and otherwise clear privateStorageNew then publicStorageNew,
ensuring private is attempted even if public throws, or (B) if legacy clear is
intentionally unsupported, add an explicit
check(require(!useOldStorage))/check(...) that fails fast with an actionable
message; also update the public KDoc for clearAllCaches() to include the brief
description and the standard "[Report a problem]" link.
In
`@ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt`:
- Around line 116-119: The public suspend fun clear() in the CacheStorage
interface currently has an empty KDoc and a no-op default implementation; update
its KDoc to include the standard "[Report a problem]" link and explicitly
document that the default implementation is a no-op and stateful CacheStorage
implementations must override clear() so that HttpCache.clearAllCaches()
actually clears stateful caches; after updating the KDoc and/or implementation,
run the legacy ABI update task to refresh the public API signature file.
In
`@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt`:
- Around line 121-130: clear() currently deletes files and then clears the
mutexes map while other ops (readCache, updateCache, deleteCache) can still be
running, causing race conditions; introduce a shared synchronization primitive
(e.g., a global Mutex or a read/write lock named something like globalCacheLock)
and modify readCache, updateCache, and deleteCache to acquire a shared/read lock
(or lock.withLock) while clear() acquires the exclusive/write lock so clear()
runs serialized with all file ops; in clear(), hold that exclusive lock for the
entire deletion phase, check the boolean return of file.delete() and log/warn or
handle failures (do not silently ignore false), and only call mutexes.clear()
while still holding the exclusive lock after all deletions complete.
- Around line 74-77: The clear() implementation updates delegate and the
in-memory store in two unsynchronized steps, allowing concurrent
find/findAll/remove/removeAll to observe stale data; fix it by protecting store
and all accessors with a single coroutine Mutex (e.g., add a private val mutex =
Mutex() in FileCacheStorage) and wrap delegate/store mutations and reads in
mutex.withLock { ... } — apply withLock in clear(), find(), findAll(), remove(),
and removeAll() so clear becomes atomic with respect to other operations.
In `@ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt`:
- Line 85: The test function name testClear is not descriptive and should follow
the repository pattern using a backticked, human-readable description; rename
the function (e.g., change fun testClear() = runTest { ... } to something like
fun `clears stored files from FileStorage`() = runTest { ... }) and update any
references to that function if present (keep the runTest body unchanged and
ensure the function is top-level test function as before).
---
Nitpick comments:
In `@ktor-client/ktor-client-core/api/ktor-client-core.api`:
- Around line 636-690: Update the KDoc for the interface method
CacheStorage.clear() to state that the default implementation is a no-op and
does not perform cleanup, and explicitly advise implementors to override clear()
to perform any necessary resource or entry cleanup; reference the interface
method CacheStorage.clear and the provided default implementation
CacheStorage$DefaultImpls.clear, and mention common implementations
(InMemoryCacheStorage, CachingCacheStorage, FileCacheStorage, UnlimitedStorage)
as examples that should implement proper cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a9b660e-e7f0-4150-a09c-f0fbf477a56d
📒 Files selected for processing (8)
ktor-client/ktor-client-core/api/ktor-client-core.apiktor-client/ktor-client-core/api/ktor-client-core.klib.apiktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/UnlimitedCacheStorage.ktktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.ktktor-client/ktor-client-core/jvm/test/CachingCacheStorageTest.ktktor-client/ktor-client-core/jvm/test/FileStorageTest.kt
| } | ||
|
|
||
| final class io.ktor.client.plugins.cache/HttpCache { // io.ktor.client.plugins.cache/HttpCache|null[0] | ||
| final suspend fun clearAllCaches() // io.ktor.client.plugins.cache/HttpCache.clearAllCaches|clearAllCaches(){}[0] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify clearAllCaches behavior for both new CacheStorage and legacy HttpCacheStorage paths.
rg -n -C4 'fun clearAllCaches|useOldStorage|publicStorageNew|privateStorageNew|publicStorage: HttpCacheStorage|privateStorage: HttpCacheStorage' \
--glob '*.kt'
rg -n -C4 'abstract class HttpCacheStorage|interface CacheStorage|fun clear|override .*clear' \
--glob '*.kt'Repository: ktorio/ktor
Length of output: 50367
🏁 Script executed:
# Find HttpCacheStorage class definition and check for clear method
rg -n 'abstract class HttpCacheStorage|interface HttpCacheStorage|class.*HttpCacheStorage' --glob '*.kt' -A 20Repository: ktorio/ktor
Length of output: 5879
🏁 Script executed:
# Check what methods are in HttpCacheStorage
rg -n 'class HttpCacheStorage|abstract.*HttpCacheStorage' --glob '*.kt' -A 50 | head -100Repository: ktorio/ktor
Length of output: 7036
🏁 Script executed:
# Look for CacheStorage interface to understand the new API
rg -n 'interface CacheStorage|class.*CacheStorage' --glob '*.kt' -A 15 | grep -E '(interface|class|fun|suspend)' | head -50Repository: ktorio/ktor
Length of output: 4129
🏁 Script executed:
# Get the full CacheStorage interface definition
cat -n ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt | sed -n '79,150p'Repository: ktorio/ktor
Length of output: 3162
🏁 Script executed:
# Check the legacy interceptor implementations to see if they use publicStorage/privateStorage
rg -n 'fun interceptSendLegacy|fun interceptReceiveLegacy' --glob '*.kt' -A 30 | head -80Repository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Also check if HttpCacheStorage has clear method anywhere
rg -n 'abstract fun clear|override fun clear|fun clear' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.ktRepository: ktorio/ktor
Length of output: 92
🏁 Script executed:
# Search for the legacy interceptor methods in HttpCache.kt
rg -n 'interceptSendLegacy|interceptReceiveLegacy' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt -A 25Repository: ktorio/ktor
Length of output: 2879
🏁 Script executed:
# Find the actual implementations of interceptSendLegacy and interceptReceiveLegacy
rg -n 'private.*fun interceptSendLegacy|private.*fun interceptReceiveLegacy' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt -A 40 | head -120Repository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Search more broadly for these legacy methods
rg -n 'interceptSendLegacy|interceptReceiveLegacy' --glob '*.kt' -B 2 -A 20Repository: ktorio/ktor
Length of output: 12198
🏁 Script executed:
# Look at cacheResponse method to see which storage it uses
rg -n 'fun cacheResponse|private.*fun cacheResponse' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt -A 20Repository: ktorio/ktor
Length of output: 1039
🏁 Script executed:
# Check if publicStorage or privateStorage are ever READ (not just stored)
rg -n 'plugin\.publicStorage\.|plugin\.privateStorage\.|\.publicStorage\.|\.privateStorage\.' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/ --glob '*.kt' -B 2 -A 2Repository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Check all references to publicStorage and privateStorage in the cache plugin
rg -n 'publicStorage|privateStorage' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/ --glob '*.kt'Repository: ktorio/ktor
Length of output: 4749
🏁 Script executed:
# Get the full context of the legacy storage access in HttpCacheLegacy.kt
cat -n ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt | sed -n '100,170p'Repository: ktorio/ktor
Length of output: 3174
clearAllCaches() does not clear legacy cache storages when useOldStorage is enabled.
When the deprecated publicStorage or privateStorage properties are set, useOldStorage becomes true and the legacy interceptor path is activated. This path uses the deprecated HttpCacheStorage properties for all cache operations (lines 114, 120, 128, 131, 162 in HttpCacheLegacy.kt). However, clearAllCaches() only clears publicStorageNew and privateStorageNew, leaving stale entries in the actual legacy storages that are being actively read from.
Additionally, HttpCacheStorage lacks a clear() method to support clearing, making it impossible to properly support the legacy storage path without API changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ktor-client/ktor-client-core/api/ktor-client-core.klib.api` at line 413,
clearAllCaches() currently only clears publicStorageNew/privateStorageNew and
misses legacy storages when useOldStorage is true; to fix, add a clear() method
to the legacy HttpCacheStorage interface/implementation and update
clearAllCaches() to detect useOldStorage (or non-null
publicStorage/privateStorage) and call
publicStorage.clear()/privateStorage.clear() for the legacy path, while still
clearing publicStorageNew/privateStorageNew for the modern path; also ensure
HttpCacheLegacy.kt code paths that use publicStorage/privateStorage rely on that
new clear() and mark the API change with deprecation notes for the old
properties.
Comment on lines
+116
to
+119
| /** | ||
| * Removes all entries from this cache storage. | ||
| */ | ||
| public suspend fun clear() {} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the public API signature files include the new CacheStorage.clear and HttpCache.clearAllCaches APIs.
./gradlew :ktor-client:ktor-client-core:checkLegacyAbiRepository: ktorio/ktor
Length of output: 5579
🏁 Script executed:
cat -n ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt | sed -n '100,140p'Repository: ktorio/ktor
Length of output: 1708
🏁 Script executed:
# Check KDoc patterns in other public methods in the same file
rg -A 3 'public suspend fun' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.ktRepository: ktorio/ktor
Length of output: 1112
🏁 Script executed:
# Find HttpCache.clearAllCaches() to understand the usage context
fd -t f 'HttpCache' ktor-client/ktor-client-core --type f -name '*.kt' | head -20Repository: ktorio/ktor
Length of output: 285
🏁 Script executed:
# Check for ABI signature files in the module
find ktor-client/ktor-client-core -type f -name '*.txt' | grep -i apiRepository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Check if CacheStorage is an interface
rg -B 10 'public suspend fun clear' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt | head -20Repository: ktorio/ktor
Length of output: 397
🏁 Script executed:
# Find HttpCache class and clearAllCaches() method
fd HttpCache.kt ktor-client/ktor-client-core --type fRepository: ktorio/ktor
Length of output: 136
🏁 Script executed:
# Look for API signature files
find ktor-client/ktor-client-core -type d -name apiRepository: ktorio/ktor
Length of output: 154
🏁 Script executed:
# Check the class/interface declaration of CacheStorage
head -50 ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.ktRepository: ktorio/ktor
Length of output: 1703
🏁 Script executed:
# Find and read HttpCache.clearAllCaches() method
rg -A 10 'fun clearAllCaches' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.ktRepository: ktorio/ktor
Length of output: 478
🏁 Script executed:
# List ABI api files
ls -la ktor-client/ktor-client-core/api/Repository: ktorio/ktor
Length of output: 324
🏁 Script executed:
# Find the new CacheStorage interface (not deprecated HttpCacheStorage)
rg 'interface CacheStorage' ktor-client/ktor-client-core/common/srcRepository: ktorio/ktor
Length of output: 183
🏁 Script executed:
# Check if clear() is in the ABI file already
grep -A 2 'public suspend fun clear' ktor-client/ktor-client-core/api/ktor-client-core.apiRepository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Verify KDoc pattern for public API by checking if clear() should have the report link
rg '\[Report a problem\]' ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt | wc -lRepository: ktorio/ktor
Length of output: 57
🏁 Script executed:
# Check current state of ktor-client-core.api to confirm clear() absence
head -100 ktor-client/ktor-client-core/api/ktor-client-core.api | grep -i "clear\|CacheStorage"Repository: ktorio/ktor
Length of output: 37
Add public API report link and document the default no-op behavior.
The KDoc is missing the standard [Report a problem] link that all other public methods in this interface include. Additionally, the empty default implementation means custom CacheStorage implementations that don't override clear() will silently do nothing when HttpCache.clearAllCaches() is called. Document that stateful implementations must override this method, and update the ABI signature file.
Suggested KDoc fix
/**
* Removes all entries from this cache storage.
+ *
+ * The default implementation is a no-op. Stateful implementations must override this method.
+ *
+ * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.client.plugins.cache.storage.CacheStorage.clear)
*/
public suspend fun clear() {}Also run ./gradlew :ktor-client:ktor-client-core:updateLegacyAbi to update the public API signature file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt`
around lines 116 - 119, The public suspend fun clear() in the CacheStorage
interface currently has an empty KDoc and a no-op default implementation; update
its KDoc to include the standard "[Report a problem]" link and explicitly
document that the default implementation is a no-op and stateful CacheStorage
implementations must override clear() so that HttpCache.clearAllCaches()
actually clears stateful caches; after updating the KDoc and/or implementation,
run the legacy ABI update task to refresh the public API signature file.
Comment on lines
+121
to
+130
| override suspend fun clear(): Unit = withContext(dispatcher) { | ||
| val files = directory.listFiles() ?: return@withContext | ||
| for (file in files) { | ||
| try { | ||
| file.delete() | ||
| } catch (cause: Exception) { | ||
| LOGGER.trace { "Exception during cache deletion in a file: ${cause.stackTraceToString()}" } | ||
| } | ||
| } | ||
| mutexes.clear() |
Contributor
There was a problem hiding this comment.
Do not clear file mutexes while file operations can still be in flight.
readCache, updateCache, and deleteCache create per-key mutexes with computeIfAbsent, but clear() deletes files without taking those locks and then wipes mutexes. A racing writer can keep using an old mutex and recreate a cache file after clear() returns, while later operations use a new mutex for the same key. Serialize clear() with all file operations using a shared mutex, and also handle file.delete() returning false so undeleted cache files are not silently retained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt`
around lines 121 - 130, clear() currently deletes files and then clears the
mutexes map while other ops (readCache, updateCache, deleteCache) can still be
running, causing race conditions; introduce a shared synchronization primitive
(e.g., a global Mutex or a read/write lock named something like globalCacheLock)
and modify readCache, updateCache, and deleteCache to acquire a shared/read lock
(or lock.withLock) while clear() acquires the exclusive/write lock so clear()
runs serialized with all file ops; in clear(), hold that exclusive lock for the
entire deletion phase, check the boolean return of file.delete() and log/warn or
handle failures (do not silently ignore false), and only call mutexes.clear()
while still holding the exclusive lock after all deletions complete.
Contributor
There was a problem hiding this comment.
+1 here. Looks like you'll need a global read-write lock and wait for the mutexes to drain in order to avoid trying to delete locked files.
Contributor
There was a problem hiding this comment.
@bjhham, agreed — a global read-write lock is the right approach here. Since all file I/O is already dispatched to withContext(dispatcher) (i.e., Dispatchers.IO), using java.util.concurrent.locks.ReentrantReadWriteLock is appropriate; blocking on an IO thread is expected and acceptable.
The key restructuring needed:
readCache,updateCache, anddeleteCacheacquire the read lock around their entirewithContextbody (keeping the per-key coroutineMutexfor URL-level serialization inside).clear()acquires the write lock, blocking until all in-flight read-lock holders drain, then deletes files and clearsmutexesbefore releasing.file.delete()returningfalseshould be explicitly logged rather than silently ignored.
Here's a concrete suggestion:
import java.util.concurrent.locks.ReentrantReadWriteLock
import kotlin.concurrent.read
import kotlin.concurrent.write
private class FileCacheStorage(
private val directory: File,
private val dispatcher: CoroutineDispatcher = Dispatchers.IO
) : CacheStorage {
private val mutexes = ConcurrentMap<String, Mutex>()
private val globalLock = ReentrantReadWriteLock()
// ... init, key(), etc. unchanged ...
override suspend fun store(url: Url, data: CachedResponseData): Unit = withContext(dispatcher) {
val urlHex = key(url)
globalLock.readLock().lock()
try {
updateCache(urlHex) { caches ->
caches.filterNot { it.varyKeys == data.varyKeys } + data
}
} finally {
globalLock.readLock().unlock()
}
}
override suspend fun findAll(url: Url): Set<CachedResponseData> = withContext(dispatcher) {
globalLock.readLock().lock()
try {
readCache(key(url)).toSet()
} finally {
globalLock.readLock().unlock()
}
}
override suspend fun find(url: Url, varyKeys: Map<String, String>): CachedResponseData? = withContext(dispatcher) {
globalLock.readLock().lock()
try {
val data = readCache(key(url))
data.find { varyKeys.all { (key, value) -> it.varyKeys[key] == value } }
} finally {
globalLock.readLock().unlock()
}
}
override suspend fun remove(url: Url, varyKeys: Map<String, String>) = withContext(dispatcher) {
val urlHex = key(url)
globalLock.readLock().lock()
try {
updateCache(urlHex) { caches -> caches.filterNot { it.varyKeys == varyKeys } }
} finally {
globalLock.readLock().unlock()
}
}
override suspend fun removeAll(url: Url) = withContext(dispatcher) {
val urlHex = key(url)
globalLock.readLock().lock()
try {
deleteCache(urlHex)
} finally {
globalLock.readLock().unlock()
}
}
override suspend fun clear(): Unit = withContext(dispatcher) {
globalLock.writeLock().lock()
try {
val files = directory.listFiles() ?: return@withContext
for (file in files) {
try {
if (!file.delete()) {
LOGGER.trace { "Failed to delete cache file: ${file.path}" }
}
} catch (cause: Exception) {
LOGGER.trace { "Exception during cache deletion in a file: ${cause.stackTraceToString()}" }
}
}
mutexes.clear()
} finally {
globalLock.writeLock().unlock()
}
}
// readCache, updateCache, deleteCache remain unchanged internally
// (per-key Mutex still guards URL-level serialization within a read-locked section)
}A few notes on this approach:
- Draining:
writeLock().lock()will block until all activereadLock()holders finish — this is exactly the "wait for mutexes to drain" semantic the maintainer asked for. - No suspension within lock: The
readLock/writeLockis acquired before calling thesuspendhelpers. Since we're already insidewithContext(dispatcher)on an IO thread, blocking on the Java lock is safe and does not risk pinning a limited thread pool. mutexes.clear()inside the write lock: Ensures no new operation can pick up a stale mutex reference afterclear()returns.file.delete()return value: Now explicitly checked and traced.
🧠 Learnings used
Learnt from: dapzthelegend
Repo: ktorio/ktor PR: 5504
File: ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt:45-54
Timestamp: 2026-04-12T16:11:51.468Z
Learning: In ktorio/ktor, `ConcurrentMapNative` (ktor-utils/posix/src/io/ktor/util/collections/ConcurrentMapNative.kt) intentionally returns snapshot copies (LinkedHashSet/ArrayList) for `entries`, `keys`, and `values` properties instead of live delegate views. This is by design to prevent ConcurrentModificationException on Native/Posix when iteration occurs outside the lock (see issue `#5480`). JVM and Web targets use their platform map implementations and are unaffected. Do not flag this Native-only snapshot behavior as a cross-platform divergence issue.
Learnt from: rururux
Repo: ktorio/ktor PR: 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
Repo: ktorio/ktor PR: 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
Repo: ktorio/ktor PR: 5345
File: ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RouteSelector.kt:694-699
Timestamp: 2026-02-05T10:32:08.803Z
Learning: In Kotlin sources, avoid annotating declarations with InternalAPI when they already have internal visibility, because internal already restricts access within the module. Apply this guidance across all Kotlin files (pattern **/*.kt). If a declaration is intended to be part of a public or exported API, or if InternalAPI conveys additional semantics beyond visibility, consider keeping the annotation and documenting the rationale.
Learnt from: nomisRev
Repo: ktorio/ktor PR: 5449
File: ktor-server/ktor-server-core/common/src/io/ktor/server/config/ApplicationConfig.kt:160-179
Timestamp: 2026-03-18T08:26:32.639Z
Learning: In the ktorio/ktor repository, apply a minimal KDoc style guideline for public API extension functions: include a brief description and a [Report a problem] link, and do not require explicit param, return, or throws tags. Do not flag missing KDoc param/return/throws as issues in this project; this guideline should apply to Kotlin source files across the codebase where public API surface is documented.
bjhham
requested changes
Apr 20, 2026
Contributor
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
The AI had some good points regarding thread safety here. Introducing the bulk delete can complicate matters.
Comment on lines
+121
to
+130
| override suspend fun clear(): Unit = withContext(dispatcher) { | ||
| val files = directory.listFiles() ?: return@withContext | ||
| for (file in files) { | ||
| try { | ||
| file.delete() | ||
| } catch (cause: Exception) { | ||
| LOGGER.trace { "Exception during cache deletion in a file: ${cause.stackTraceToString()}" } | ||
| } | ||
| } | ||
| mutexes.clear() |
Contributor
There was a problem hiding this comment.
+1 here. Looks like you'll need a global read-write lock and wait for the mutexes to drain in order to avoid trying to delete locked files.
Contributor
Author
|
Pushed
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt`:
- Around line 88-93: The current pattern in FileCacheStorage.kt where clear()
tries to acquire all permits from semaphore (and relies on MAX_PERMITS) can
deadlock when two clears interleave; replace that pattern by serializing clear()
with a dedicated mutex or by using a ReentrantReadWriteLock: stop using the
“acquire all 1000 permits” approach (refer to semaphore and MAX_PERMITS) and
instead add a clearMutex (or a write lock on a ReentrantReadWriteLock) that
clear() obtains exclusively while regular file operations keep using the
existing concurrency control (or the read lock), ensuring clear() runs
one-at-a-time and removing the arbitrary MAX_PERMITS cap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dad75b3a-7bfe-4800-a748-a9d63011326b
📒 Files selected for processing (5)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.ktktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.ktktor-client/ktor-client-core/jvm/test/CachingCacheStorageTest.ktktor-client/ktor-client-core/jvm/test/FileStorageTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/HttpCacheStorage.kt
Contributor
Author
|
Pushed Added a dedicated |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (1)
88-96: Heads-up:MAX_PERMITS = 1000is a new hard cap on concurrent file ops.Prior to this change,
FileCacheStoragehad no upper bound on concurrent in-flight requests; now any 1001st concurrentstore/find/findAll/removewill suspend until a permit frees. 1000 is high enough that it's unlikely to bite in practice, but the choice is unjustified in code and could surprise heavy users.Two low-effort options worth considering:
- Add a brief comment explaining the rationale (the cap exists solely so
clear()can drain in-flight ops by acquiring all permits).- Or replace the semaphore-as-write-lock pattern with
java.util.concurrent.locks.ReentrantReadWriteLock(read lock around per-op work, write lock inclear()), which removes the magic 1000, gives writer fairness for free, and avoids the suspendingrepeat(1000) { acquire() }loop inclear(). Blocking on a JDK lock is fine here since all access is already onwithContext(dispatcher).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt` around lines 88 - 96, The change introduced a hard cap MAX_PERMITS = 1000 and a Semaphore(MAX_PERMITS) used by FileCacheStorage (semaphore, clear(), clearMutex) which can unexpectedly throttle the 1001st concurrent file op; update the code by adding a concise comment next to MAX_PERMITS and the semaphore explaining the rationale (the cap exists solely so clear() can drain in-flight ops by acquiring all permits to avoid deadlock when splitting permits) and note the tradeoffs, or alternatively replace the semaphore/clearMutex pattern with a java.util.concurrent.locks.ReentrantReadWriteLock (use readLock around per-op methods like store/find/findAll/remove and writeLock in clear()) to avoid a magic numeric cap and the repeat(1000){ acquire() } loop — reference symbols: MAX_PERMITS, semaphore, clear(), clearMutex, FileCacheStorage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt`:
- Around line 88-96: The change introduced a hard cap MAX_PERMITS = 1000 and a
Semaphore(MAX_PERMITS) used by FileCacheStorage (semaphore, clear(), clearMutex)
which can unexpectedly throttle the 1001st concurrent file op; update the code
by adding a concise comment next to MAX_PERMITS and the semaphore explaining the
rationale (the cap exists solely so clear() can drain in-flight ops by acquiring
all permits to avoid deadlock when splitting permits) and note the tradeoffs, or
alternatively replace the semaphore/clearMutex pattern with a
java.util.concurrent.locks.ReentrantReadWriteLock (use readLock around per-op
methods like store/find/findAll/remove and writeLock in clear()) to avoid a
magic numeric cap and the repeat(1000){ acquire() } loop — reference symbols:
MAX_PERMITS, semaphore, clear(), clearMutex, FileCacheStorage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec07ee5a-80b6-4962-af9e-46e65293dff2
📒 Files selected for processing (1)
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Subsystem
Client, HttpCache
Motivation
Fixes #4719. There is no way to clear the HttpCache, including the in-memory layer in
CachingCacheStorage. Deleting cache files manually still leaves stale in-memory entries.Solution
Add
suspend fun clear()toCacheStorage(default no-op) with overrides inUnlimitedStorage,CachingCacheStorage, andFileCacheStorage. ExposeHttpCache.clearAllCaches()so callers can doclient.plugin(HttpCache).clearAllCaches().