Add clear API to HttpCache for clearing all cached data#5535
Open
fru1tworld wants to merge 4 commits into
Open
Add clear API to HttpCache for clearing all cached data#5535fru1tworld wants to merge 4 commits into
fru1tworld wants to merge 4 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds suspend-based cache purge APIs: ChangesCache clear flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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.
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.
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
- HttpCache.clearAllCaches always attempts both private and public storages; if both fail, the first failure is thrown with the second attached as a suppressed cause. Cancellation propagates immediately. - FileCacheStorage replaces the bounded semaphore + clearMutex pattern with a coroutine-safe read/write coordination via a single Mutex (held by clear() for its full duration) and an atomic reader counter that signals drain through a CONFLATED channel. Removes the magic 1000-permit cap.
Contributor
Author
|
Addressed the read-write coordination feedback in 4f1162c. PTAL. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (1)
145-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle exceptions from
file.delete()to stay consistent withdeleteCacheand ensuremutexes.clear()runs.
File.delete()can throw (e.g.,SecurityException) —deleteCache(Lines 190‑194) already catches and traces this. Inclear()an unchecked exception from one file aborts the loop, skips the remaining files, and skipsmutexes.clear(), leaving stale per-URL mutexes behind. Mirror thedeleteCachepattern so a single problematic file does not poison the whole purge.🛡️ Suggested fix
val files = directory.listFiles() ?: return@withLock for (file in files) { - if (!file.delete()) { - LOGGER.trace { "Failed to delete cache file: ${file.name}" } + try { + if (!file.delete()) { + LOGGER.trace { "Failed to delete cache file: ${file.name}" } + } + } catch (cause: Exception) { + LOGGER.trace { "Exception during cache deletion in a file: ${cause.stackTraceToString()}" } } } mutexes.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt` around lines 145 - 152, The clear() implementation iterates files and calls file.delete() without catching exceptions, so a thrown SecurityException can abort deletion and prevent mutexes.clear() from running; mirror the deleteCache(...) pattern by wrapping each file.delete() call in a try/catch that logs via LOGGER.trace (including the exception) on failure and continues, ensuring the loop completes and mutexes.clear() always executes; locate the clear() function in FileCacheStorage.kt and use the same logging/message style used by deleteCache to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt`:
- Around line 145-152: The clear() implementation iterates files and calls
file.delete() without catching exceptions, so a thrown SecurityException can
abort deletion and prevent mutexes.clear() from running; mirror the
deleteCache(...) pattern by wrapping each file.delete() call in a try/catch that
logs via LOGGER.trace (including the exception) on failure and continues,
ensuring the loop completes and mutexes.clear() always executes; locate the
clear() function in FileCacheStorage.kt and use the same logging/message style
used by deleteCache to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65400abf-3557-4b81-b828-2cac461b40d2
📒 Files selected for processing (2)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.ktktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.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/HttpCache.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().