Skip to content

Exclusive Cache Lock #8209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
8674a8b
Cache Lock
yschimke Jan 20, 2024
c3df60f
Merge branch 'master' into cache_singleton
yschimke Apr 1, 2024
166a255
Merge branch 'master' into cache_singleton
yschimke Apr 1, 2024
b02738d
Rework
yschimke Apr 1, 2024
ac0cc79
Rework
yschimke Apr 1, 2024
2525edf
Add test
yschimke Apr 1, 2024
7072358
Cleanup
yschimke Apr 1, 2024
5ca131f
Activate missed test
yschimke Apr 1, 2024
0a2fbb6
Cleanup
yschimke Apr 1, 2024
c9db9d4
Avoid checking on non system file systems
yschimke Apr 1, 2024
97e566b
Revert "Avoid checking on non system file systems"
yschimke Apr 1, 2024
ee1daa6
Avoid failing on dns tests
yschimke Apr 1, 2024
3041258
Adding a test
yschimke Apr 2, 2024
7faba3e
Fixes
yschimke Apr 2, 2024
44d3640
Test on more platforms
yschimke Apr 3, 2024
792b689
Cleanup
yschimke Apr 3, 2024
5117f46
Merge branch 'master' into cache_singleton
yschimke Apr 3, 2024
a922b1d
Cleanup
yschimke Apr 3, 2024
e24e146
Fixes
yschimke Apr 6, 2024
04c057f
Fixes
yschimke Apr 6, 2024
5c6c60d
Cache fixes
yschimke Apr 6, 2024
12b7eb9
Cache fixes
yschimke Apr 6, 2024
ae3830d
Cache fixes
yschimke Apr 6, 2024
99a5ee5
Cache fixes
yschimke Apr 6, 2024
cc97769
Cache fixes
yschimke Apr 6, 2024
6dbb03f
Fix test
yschimke Apr 6, 2024
19be22b
Merge branch 'refs/heads/master' into cache_singleton
yschimke Apr 6, 2024
feafff9
Fix test
yschimke Apr 6, 2024
72359a6
avoid windows for now
yschimke Apr 6, 2024
b76aa7f
Fix or skip on windows
yschimke Apr 7, 2024
89e9d80
cleanup
yschimke Apr 7, 2024
28025e2
skip on windows
yschimke Apr 7, 2024
fa707bc
Update DuplexTest.kt
yschimke Apr 7, 2024
c5a87f0
Merge branch 'master' into cache_singleton
yschimke Jan 4, 2025
bb0d4b0
Fixes
yschimke Jan 4, 2025
192e93b
Fixes
yschimke Jan 4, 2025
b4115b2
Fixes
yschimke Jan 4, 2025
2f959af
Fixes
yschimke Jan 4, 2025
7af5705
Fixes
yschimke Jan 4, 2025
4e9d4b3
Fix test
yschimke Jan 11, 2025
019560c
reformat
yschimke Jan 11, 2025
433f6f7
Make it optional
yschimke Jan 11, 2025
b650c9a
Merge branch 'master' into cache_singleton
yschimke May 10, 2025
f9479e8
reformat
yschimke May 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Cache fixes
  • Loading branch information
yschimke committed Apr 6, 2024
commit 99a5ee5f3ccedb93f7e7032c71ecf6958c37320f
19 changes: 15 additions & 4 deletions okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,20 @@ import okio.Path
import okio.Path.Companion.toOkioPath
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement

/**
* An implementation of directory locking to ensure exclusive access in a Cache instance.
* Always applies for the current process, and uses a file system lock if supported.
*/
internal object CacheLock {
private val openCaches = Collections.synchronizedMap(mutableMapOf<Path, Exception>())

/**
* Open a lock, which if successful remains until the returned [Closeable] is closed.
* The lock file will be a file with name "lock" inside directory.
*
* @param fileSystem the file system containing the lock files.
* @param directory the cache directory.
*/
fun openLock(
fileSystem: FileSystem,
directory: Path,
Expand All @@ -44,10 +55,10 @@ internal object CacheLock {
memoryLock.close()
fileSystemLock.close()
}
} catch (e: Exception) {
} catch (le: LockException) {
if (fileSystemSupportsLock(fileSystem)) {
memoryLock.close()
throw e
throw le
}
}
}
Expand All @@ -73,7 +84,7 @@ internal object CacheLock {
*/
@SuppressLint("NewApi")
@IgnoreJRERequirement // D8 supports put if absent
fun inMemoryLock(directory: Path): Closeable {
internal fun inMemoryLock(directory: Path): Closeable {
val existing = openCaches.putIfAbsent(directory, LockException("Existing CacheLock($directory) opened at"))
if (existing != null) {
throw LockException("Cache already open at '$directory' in same process", existing)
Expand All @@ -89,7 +100,7 @@ internal object CacheLock {
*/
@SuppressLint("NewApi")
@IgnoreJRERequirement // Not called on legacy Android
fun fileSystemLock(directory: Path): Closeable {
internal fun fileSystemLock(directory: Path): Closeable {
// update once https://github.com/square/okio/issues/1464 is available

val lockFile = directory / "lock"
Expand Down
27 changes: 25 additions & 2 deletions okhttp/src/test/java/okhttp3/CacheLockTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import okio.Closeable
import okio.FileSystem
import okio.Path.Companion.toOkioPath
import okio.Path.Companion.toPath
import okio.SYSTEM
import okio.fakefilesystem.FakeFileSystem
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Assumptions.assumeTrue
import org.junit.jupiter.api.BeforeEach
Expand Down Expand Up @@ -138,8 +140,29 @@ class CacheLockTest {
}
}

private fun openCache(directory: okio.Path): Cache {
return Cache(directory, 10_000, FileSystem.SYSTEM).apply {
@Test
fun testCacheLockOnFakeFileSystem() {
val fileSystem = FakeFileSystem()
val fakeCacheDir = "/missing".toPath()

val cache = openCache(fakeCacheDir, fileSystem)

val lockException =
assertThrows<LockException> {
openCache(fakeCacheDir, fileSystem)
}
assertThat(lockException.message).isEqualTo("Cache already open at '$fakeCacheDir' in same process")

cache.close()

openCache(fakeCacheDir, fileSystem)
}

private fun openCache(
directory: okio.Path,
fileSystem: FileSystem = FileSystem.SYSTEM,
): Cache {
return Cache(directory, 10_000, fileSystem).apply {
// force early LRU initialisation
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m a bit torn on this. You don’t get a failure immediately; you get it eventually.

This is probably fine; this new feature is advisory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this currently works until it doesn't. Then it ends up as 40+ bugs raised.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't really do IO operations on the main thread, so this should always be the case.

Which reminds me, for SSL init on main thread #8248

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with ‘We can’t really do I/O operations on the main thread’, but I don’t agree that OkHttpClient is being initialized on the main thread. We’re an I/O library!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the dominant DI libraries will all be initializing OkHttp on the main thread. You have to do extra work to avoid that.

initialize()
toClose.add(this)
Expand Down