Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix cache file open bugs #7625

Merged
merged 13 commits into from
Jan 8, 2023
Merged

Fix cache file open bugs #7625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7499226
Testing cache closes
yschimke Jan 3, 2023
e499080
More test fixes
yschimke Jan 3, 2023
e263edd
More test fixes
yschimke Jan 3, 2023
b83b9eb
More test fixes
yschimke Jan 3, 2023
5ff3cac
Fix more tests
yschimke Jan 4, 2023
6c9a8b4
Fix more tests
yschimke Jan 4, 2023
dea78fa
Fix tests
yschimke Jan 4, 2023
f9789c1
Merge branch 'master' into check_cache_closes
yschimke Jan 5, 2023
3fee79d
Review comments
yschimke Jan 5, 2023
dc5c35e
Cleanup
yschimke Jan 5, 2023
91bb3d4
Reverts
yschimke Jan 5, 2023
ee8adcd
Merge branch 'master' into check_cache_closes
yschimke Jan 8, 2023
4ef928b
Okio use
yschimke Jan 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion okhttp/src/jvmMain/kotlin/okhttp3/internal/cache/DiskLruCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ class DiskLruCache(
}
} catch (_: IOException) {
mostRecentRebuildFailed = true
journalWriter?.closeQuietly()
journalWriter = blackholeSink().buffer()
}

Expand Down Expand Up @@ -301,6 +302,7 @@ class DiskLruCache(
if (!exhausted()) {
rebuildJournal()
} else {
journalWriter?.closeQuietly()
journalWriter = newJournalWriter()
}
}
Expand Down Expand Up @@ -423,6 +425,7 @@ class DiskLruCache(
fileSystem.atomicMove(journalFileTmp, journalFile)
}

journalWriter?.closeQuietly()
journalWriter = newJournalWriter()
hasJournalErrors = false
mostRecentRebuildFailed = false
Expand Down Expand Up @@ -688,7 +691,7 @@ class DiskLruCache(
}

trimToSize()
journalWriter!!.close()
journalWriter?.closeQuietly()
journalWriter = null
closed = true
}
Expand Down
3 changes: 0 additions & 3 deletions okhttp/src/jvmMain/kotlin/okhttp3/internal/cache/FaultHidingSink.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ internal open class FaultHidingSink(
}

override fun close() {
if (hasErrors) {
return
}
try {
super.close()
} catch (e: IOException) {
Expand Down
50 changes: 32 additions & 18 deletions okhttp/src/jvmTest/java/okhttp3/internal/cache/DiskLruCacheTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ class DiskLruCacheTest {
}

private fun createNewCacheWithSize(maxSize: Int) {
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, maxSize.toLong(), taskRunner)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, maxSize.toLong(), taskRunner).also {
toClose.add(it)
}
synchronized(cache) { cache.initialize() }
toClose.add(cache)
}

fun setUp(baseFilesystem: FileSystem, windows: Boolean) {
Expand All @@ -95,8 +96,7 @@ class DiskLruCacheTest {
}
taskFaker.close()

// TODO uncomment after fixing up snapshot calls in this test
// (filesystem.delegate as? FakeFileSystem)?.checkNoOpenFiles()
(filesystem.delegate as? FakeFileSystem)?.checkNoOpenFiles()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the goal of this review.

Copy link
Member

Choose a reason for hiding this comment

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

Nice

}

@ParameterizedTest
Expand All @@ -122,8 +122,9 @@ class DiskLruCacheTest {
// Simulate a severe Filesystem failure on the first initialization.
filesystem.setFaultyDelete(cacheDir / "k1.0.tmp", true)
filesystem.setFaultyDelete(cacheDir, true)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
toClose.add(cache)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
}
try {
cache["k1"]
fail("")
Expand Down Expand Up @@ -1134,7 +1135,9 @@ class DiskLruCacheTest {
setUp(parameters.first, parameters.second)
cache.close()
val dir = (cacheDir / "testOpenCreatesDirectoryIfNecessary").also { filesystem.createDirectories(it) }
cache = DiskLruCache(filesystem, dir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
cache = DiskLruCache(filesystem, dir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
Copy link
Member

Choose a reason for hiding this comment

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

I like this idiom!

}
set("a", "a", "a")
assertThat(filesystem.exists(dir / "a.0")).isTrue()
assertThat(filesystem.exists(dir / "a.1")).isTrue()
Expand Down Expand Up @@ -1200,11 +1203,12 @@ class DiskLruCacheTest {
cache.close()
createNewCacheWithSize(10)
set("a", "aa", "aaa") // size 5
val snapshot = cache["a"]!!
set("b", "bb", "bbb") // size 5
set("c", "cc", "ccc") // size 5; will evict 'A'
cache.flush()
assertThat(snapshot.edit()).isNull()
cache["a"]!!.use {
set("b", "bb", "bbb") // size 5
set("c", "cc", "ccc") // size 5; will evict 'A'
cache.flush()
assertThat(it.edit()).isNull()
}
}

@ParameterizedTest
Expand Down Expand Up @@ -1530,8 +1534,9 @@ class DiskLruCacheTest {
fun isClosed_uninitializedCache(parameters: Pair<FileSystem, Boolean>) {
setUp(parameters.first, parameters.second)
// Create an uninitialized cache.
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
toClose.add(cache)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
}
assertThat(cache.isClosed()).isFalse()
cache.close()
assertThat(cache.isClosed()).isTrue()
Expand All @@ -1554,7 +1559,9 @@ class DiskLruCacheTest {

// Confirm that the fault didn't corrupt entries stored before the fault was introduced.
cache.close()
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
}
assertValue("a", "a", "a")
assertValue("b", "b", "b")
assertAbsent("c")
Expand Down Expand Up @@ -1585,7 +1592,9 @@ class DiskLruCacheTest {

// Confirm that the fault didn't corrupt entries stored before the fault was introduced.
cache.close()
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
}
assertValue("a", "a", "a")
assertValue("b", "b", "b")
assertAbsent("c")
Expand All @@ -1612,7 +1621,9 @@ class DiskLruCacheTest {

// Confirm that the fault didn't corrupt entries stored before the fault was introduced.
cache.close()
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
}
assertValue("a", "a", "a")
assertValue("b", "b", "b")
assertAbsent("c")
Expand All @@ -1633,7 +1644,9 @@ class DiskLruCacheTest {
// Confirm that the entry was still removed.
filesystem.setFaultyWrite(journalFile, false)
cache.close()
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner)
cache = DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also {
toClose.add(it)
}
assertAbsent("a")
assertValue("b", "b", "b")
}
Expand Down Expand Up @@ -1973,6 +1986,7 @@ class DiskLruCacheTest {
assertThat(snapshotAfterCommit.hasNext()).withFailMessage(
"Entry has been removed during creation."
).isTrue()
snapshotAfterCommit.next().close()
}

@ParameterizedTest
Expand Down