Exclusive Cache Lock #8209

yschimke · 2024-01-20T13:49:03Z

Implement a strict CacheLock that works within the process, and also via file system locking if supported.

We can expect a bunch of failures in apps that aren't strictly using a single Cache instance, but should be.

Follow up to #8083

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt

This reverts commit c9db9d4.

swankjesse

This is a good feature but I’d like a lot more integration tests before we put this into end users’ hands

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt

okhttp/src/main/kotlin/okhttp3/internal/cache/DiskLruCache.kt

okhttp/src/test/java/okhttp3/CacheLockTest.kt

swankjesse · 2024-04-02T02:37:43Z

okhttp/src/test/java/okhttp3/CacheLockTest.kt

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


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.

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

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

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!

swankjesse · 2024-04-02T02:39:38Z

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt

+ lockFile.toFile().createNewFile()
+ val channel = FileChannel.open(lockFile.toNioPath(), StandardOpenOption.APPEND)
+
+ checkNotNull(channel.tryLock()) {


I’m anxious that this is going to fail for reasons related to the host file system’s lack of support, and not because the file is positively locked.

Would it be horrible to probe the file system if a lock fails to see if EVERY lock is going to fail? Maybe we attempt to lock a file named with a random token or a timestamp? If that also fails we know it’s the file system’s fault?

A big +1 to okio solving this in a "Swanky" new API

But I'm not blocked on that.

I'll need to think more about this case.

Made an attempt. Let me know if it's suitable.

okhttp/src/test/java/okhttp3/CacheLockTest.kt

swankjesse

I like the PR!

But I’m still anxious about how it’ll work on real-world file systems. We probably should confirm it does something reasonable on Mac, Windows, Linux and Android? (Why is this off-by-default on Android?)

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt

okhttp/src/test/java/okhttp3/CacheLockTest.kt

okhttp/src/test/java/okhttp3/LockTestProgram.java

okhttp/src/test/java/okhttp3/CacheLockTest.kt

yschimke · 2024-04-03T05:30:28Z

Yep, next stage is forcing it through.

I forget why it's not on Android, I'll enable again and see. Or comment for future self.

yschimke · 2024-04-03T11:58:55Z

OK, running ok on Android.

Most tests failing validly since they don't close Cache instances with

 DiskLruCacheTest > recoverFromInitializationFailure(Pair) > [3] (FakeFileSystem, true) FAILED
    java.lang.AssertionError: Expected an exception of class java.io.IOException to be thrown, but was java.lang.IllegalStateException: Cache already open at '/cache' in same process
        at org.junit.Assert.fail(Assert.java:89)
        at kotlin.test.junit.JUnitAsserter.fail(JUnitSupport.kt:64)
        at kotlin.test.AssertionsKt__AssertionsImplKt.checkResultIsFailure(AssertionsImpl.kt:30)
        at kotlin.test.AssertionsKt.checkResultIsFailure(Unknown Source)
        at okhttp3.internal.cache.DiskLruCacheTest.recoverFromInitializationFailure(DiskLruCacheTest.kt:140)

        Caused by:
        java.lang.IllegalStateException: Cache already open at '/cache' in same process
            at okhttp3.internal.cache.CacheLock.inMemoryLock(CacheLock.kt:51)
            at okhttp3.internal.cache.CacheLock.openLock(CacheLock.kt:39)
            at okhttp3.internal.cache.DiskLruCache.initialize(DiskLruCache.kt:248)
            at okhttp3.internal.cache.DiskLruCache.get(DiskLruCache.kt:454)
            at okhttp3.internal.cache.DiskLruCacheTest.recoverFromInitializationFailure(DiskLruCacheTest.kt:141)

            Caused by:
            java.lang.Exception: CacheLock(/cache)
                at okhttp3.internal.cache.CacheLock.inMemoryLock(CacheLock.kt:49)
                at okhttp3.internal.cache.CacheLock.openLock(CacheLock.kt:39)
                at okhttp3.internal.cache.DiskLruCache.initialize(DiskLruCache.kt:248)
                at okhttp3.internal.cache.DiskLruCacheTest.createNewCacheWithSize(DiskLruCacheTest.kt:84)
                at okhttp3.internal.cache.DiskLruCacheTest.createNewCache(DiskLruCacheTest.kt:76)
                at okhttp3.internal.cache.DiskLruCacheTest.setUp(DiskLruCacheTest.kt:101)
                at okhttp3.internal.cache.DiskLruCacheTest.recoverFromInitializationFailure(DiskLruCacheTest.kt:124)

I'll fix tonight.

swankjesse · 2024-04-03T14:52:28Z

Excellent!

yschimke · 2024-04-06T15:48:11Z

Working on Windows, but tests are brittle :(

FileOperatorTest > write() FAILED
    java.io.IOException: Failed to delete temp directory C:\Users\RUNNER~1\AppData\Local\Temp\junit13626620294798743510. The following paths could not be deleted (see suppressed exceptions for details): <root>, test
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
        at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
        at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261)
        at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

Cache Lock

8674a8b

yschimke requested a review from swankjesse January 20, 2024 13:49

swankjesse reviewed Apr 1, 2024

View reviewed changes

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt Outdated Show resolved Hide resolved

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt Show resolved Hide resolved

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheLock.kt Outdated Show resolved Hide resolved

yschimke added 6 commits April 1, 2024 12:02

Merge branch 'master' into cache_singleton

c3df60f

Merge branch 'master' into cache_singleton

166a255

Rework

b02738d

Rework

ac0cc79

Add test

2525edf

Cleanup

7072358

yschimke changed the title ~~[for discussion] Exclusive Cache Lock~~ Exclusive Cache Lock Apr 1, 2024

yschimke marked this pull request as ready for review April 1, 2024 16:31

yschimke added the android Relates to usage specifically on Android label Apr 1, 2024

yschimke added 5 commits April 1, 2024 18:42

Activate missed test

5ca131f

Cleanup

0a2fbb6

Avoid checking on non system file systems

c9db9d4

Revert "Avoid checking on non system file systems"

97e566b

This reverts commit c9db9d4.

Avoid failing on dns tests

ee1daa6

swankjesse reviewed Apr 2, 2024

View reviewed changes

yschimke added 2 commits April 2, 2024 22:08

Adding a test

3041258

Fixes

7faba3e

yschimke commented Apr 2, 2024

View reviewed changes

okhttp/src/test/java/okhttp3/CacheLockTest.kt Outdated Show resolved Hide resolved

swankjesse reviewed Apr 3, 2024

View reviewed changes

Test on more platforms

44d3640

yschimke added windows Windows related jdkversions JDK 8, 17, 19 etc. labels Apr 3, 2024

Cleanup

792b689

yschimke marked this pull request as draft April 3, 2024 06:01

yschimke added 2 commits April 3, 2024 07:07

Merge branch 'master' into cache_singleton

5117f46

Cleanup

a922b1d

yschimke mentioned this pull request Apr 5, 2024

API for File Locking square/okio#1464

Open

yschimke added 6 commits April 6, 2024 09:05

Fixes

e24e146

Fixes

04c057f

Cache fixes

5c6c60d

Cache fixes

12b7eb9

Cache fixes

ae3830d

Cache fixes

99a5ee5

yschimke marked this pull request as ready for review April 6, 2024 09:00

yschimke added 5 commits April 6, 2024 10:08

Cache fixes

cc97769

Fix test

6dbb03f

Merge branch 'refs/heads/master' into cache_singleton

19be22b

Fix test

feafff9

avoid windows for now

72359a6

yschimke added 4 commits April 7, 2024 09:11

Fix or skip on windows

b76aa7f

cleanup

89e9d80

skip on windows

28025e2

Update DuplexTest.kt

fa707bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclusive Cache Lock #8209

Exclusive Cache Lock #8209

yschimke commented Jan 20, 2024 •

edited

swankjesse left a comment

swankjesse Apr 2, 2024

yschimke Apr 2, 2024

yschimke Apr 2, 2024

swankjesse Apr 3, 2024

swankjesse Apr 2, 2024

yschimke Apr 2, 2024

yschimke Apr 2, 2024

yschimke Apr 2, 2024

yschimke Apr 6, 2024

swankjesse left a comment

yschimke commented Apr 3, 2024

yschimke commented Apr 3, 2024

swankjesse commented Apr 3, 2024

yschimke commented Apr 6, 2024

Exclusive Cache Lock #8209

Are you sure you want to change the base?

Exclusive Cache Lock #8209

Conversation

yschimke commented Jan 20, 2024 • edited

swankjesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

yschimke commented Apr 3, 2024

yschimke commented Apr 3, 2024

swankjesse commented Apr 3, 2024

yschimke commented Apr 6, 2024

yschimke commented Jan 20, 2024 •

edited