diff --git a/okhttp-testing-support/src/main/kotlin/okhttp3/TestUtil.kt b/okhttp-testing-support/src/main/kotlin/okhttp3/TestUtil.kt index 8d9e61d6ed32..1f85471d244c 100644 --- a/okhttp-testing-support/src/main/kotlin/okhttp3/TestUtil.kt +++ b/okhttp-testing-support/src/main/kotlin/okhttp3/TestUtil.kt @@ -15,6 +15,7 @@ */ package okhttp3 +import java.io.File import java.net.InetAddress import java.net.InetSocketAddress import java.net.UnknownHostException @@ -42,6 +43,12 @@ object TestUtil { return String(array) } + tailrec fun File.isDescendentOf(directory: File): Boolean { + val parentFile = parentFile ?: return false + if (parentFile == directory) return true + return parentFile.isDescendentOf(directory) + } + /** * See FinalizationTester for discussion on how to best trigger GC in tests. * https://android.googlesource.com/platform/libcore/+/master/support/src/test/java/libcore/ diff --git a/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/InMemoryFileSystem.kt b/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/InMemoryFileSystem.kt index 647209f326b3..f6832d2a66af 100644 --- a/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/InMemoryFileSystem.kt +++ b/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/InMemoryFileSystem.kt @@ -19,6 +19,7 @@ import java.io.File import java.io.FileNotFoundException import java.io.IOException import java.util.IdentityHashMap +import okhttp3.TestUtil.isDescendentOf import okio.Buffer import okio.ForwardingSink import okio.ForwardingSource @@ -30,16 +31,13 @@ import org.junit.runners.model.Statement /** A simple file system where all files are held in memory. Not safe for concurrent use. */ class InMemoryFileSystem : FileSystem, TestRule { - private val files: MutableMap = mutableMapOf() - private val openSources: MutableMap = IdentityHashMap() - private val openSinks: MutableMap = IdentityHashMap() + private val files = mutableMapOf() + private val openSources = IdentityHashMap() + private val openSinks = IdentityHashMap() - override fun apply( - base: Statement, - description: Description - ): Statement { + override fun apply(base: Statement, description: Description): Statement { return object : Statement() { - @Throws(Throwable::class) override fun evaluate() { + override fun evaluate() { base.evaluate() ensureResourcesClosed() } @@ -47,32 +45,25 @@ class InMemoryFileSystem : FileSystem, TestRule { } fun ensureResourcesClosed() { - val openResources: MutableList = mutableListOf() + val openResources = mutableListOf() for (file in openSources.values) { openResources.add("Source for $file") } for (file in openSinks.values) { openResources.add("Sink for $file") } - if (!openResources.isEmpty()) { - val builder = - StringBuilder("Resources acquired but not closed:") - for (resource in openResources) { - builder.append("\n * ") - .append(resource) - } - throw IllegalStateException(builder.toString()) + check(openResources.isEmpty()) { + "Resources acquired but not closed:\n * ${openResources.joinToString(separator = "\n * ")}" } } - @Throws( - FileNotFoundException::class - ) override fun source(file: File): Source { + @Throws(FileNotFoundException::class) + override fun source(file: File): Source { val result = files[file] ?: throw FileNotFoundException() val source: Source = result.clone() openSources[source] = file return object : ForwardingSource(source) { - @Throws(IOException::class) override fun close() { + override fun close() { openSources.remove(source) super.close() } @@ -80,20 +71,12 @@ class InMemoryFileSystem : FileSystem, TestRule { } @Throws(FileNotFoundException::class) - override fun sink(file: File): Sink { - return sink(file, false) - } + override fun sink(file: File) = sink(file, false) - @Throws( - FileNotFoundException::class - ) override fun appendingSink(file: File): Sink { - return sink(file, true) - } + @Throws(FileNotFoundException::class) + override fun appendingSink(file: File) = sink(file, true) - private fun sink( - file: File, - appending: Boolean - ): Sink { + private fun sink(file: File, appending: Boolean): Sink { var result: Buffer? = null if (appending) { result = files[file] @@ -105,7 +88,7 @@ class InMemoryFileSystem : FileSystem, TestRule { val sink: Sink = result openSinks[sink] = file return object : ForwardingSink(sink) { - @Throws(IOException::class) override fun close() { + override fun close() { openSinks.remove(sink) super.close() } @@ -117,33 +100,21 @@ class InMemoryFileSystem : FileSystem, TestRule { files.remove(file) } - override fun exists(file: File): Boolean { - return files.containsKey(file) - } + override fun exists(file: File) = files.containsKey(file) - override fun size(file: File): Long { - val buffer = files[file] - return buffer?.size ?: 0L - } + override fun size(file: File) = files[file]?.size ?: 0L - @Throws(IOException::class) override fun rename( - from: File, - to: File - ) { - val buffer = files.remove(from) ?: throw FileNotFoundException() - files[to] = buffer + @Throws(IOException::class) + override fun rename(from: File, to: File) { + files[to] = files.remove(from) ?: throw FileNotFoundException() } - @Throws( - IOException::class - ) override fun deleteContents(directory: File) { - val prefix = "$directory/" + @Throws(IOException::class) + override fun deleteContents(directory: File) { val i = files.keys.iterator() while (i.hasNext()) { val file = i.next() - if (file.toString() - .startsWith(prefix) - ) i.remove() + if (file.isDescendentOf(directory)) i.remove() } } diff --git a/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/WindowsFileSystem.kt b/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/WindowsFileSystem.kt index d553dc1c585c..c07ebdd91ed1 100644 --- a/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/WindowsFileSystem.kt +++ b/okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/WindowsFileSystem.kt @@ -17,6 +17,7 @@ package okhttp3.internal.io import java.io.File import java.util.Collections +import okhttp3.TestUtil.isDescendentOf import okio.ForwardingSink import okio.ForwardingSource import okio.IOException @@ -65,12 +66,6 @@ class WindowsFileSystem(val delegate: FileSystem) : FileSystem { delegate.deleteContents(directory) } - private tailrec fun File.isDescendentOf(directory: File): Boolean { - val parentFile = parentFile ?: return false - if (parentFile == directory) return true - return parentFile.isDescendentOf(directory) - } - private inner class FileSink(val file: File, delegate: Sink) : ForwardingSink(delegate) { var closed = false diff --git a/okhttp/src/main/kotlin/okhttp3/internal/Util.kt b/okhttp/src/main/kotlin/okhttp3/internal/Util.kt index 6e2c214d686e..eccae85da25e 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/Util.kt +++ b/okhttp/src/main/kotlin/okhttp3/internal/Util.kt @@ -18,6 +18,7 @@ package okhttp3.internal import java.io.Closeable +import java.io.File import java.io.IOException import java.io.InterruptedIOException import java.net.InetSocketAddress @@ -48,6 +49,7 @@ import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.Response import okhttp3.ResponseBody.Companion.toResponseBody import okhttp3.internal.http2.Header +import okhttp3.internal.io.FileSystem import okio.Buffer import okio.BufferedSink import okio.BufferedSource @@ -514,6 +516,29 @@ fun ServerSocket.closeQuietly() { } } +/** + * Returns true if file streams can be manipulated independently of their paths. This is typically + * true for systems like Mac, Unix, and Linux that use inodes in their file system interface. It is + * typically false on Windows. + * + * If this returns false we won't permit simultaneous reads and writes. When writes commit we need + * to delete the previous snapshots, and that won't succeed if the file is open. (We do permit + * multiple simultaneous reads.) + * + * @param file a file in the directory to check. This file shouldn't already exist! + */ +fun FileSystem.isCivilized(file: File): Boolean { + sink(file).use { + try { + delete(file) + return true + } catch (_: IOException) { + } + } + delete(file) + return false +} + fun Long.toHexString(): String = java.lang.Long.toHexString(this) fun Int.toHexString(): String = Integer.toHexString(this) diff --git a/okhttp/src/main/kotlin/okhttp3/internal/cache/DiskLruCache.kt b/okhttp/src/main/kotlin/okhttp3/internal/cache/DiskLruCache.kt index 1a9bade8e7ff..93af415c0c6d 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/cache/DiskLruCache.kt +++ b/okhttp/src/main/kotlin/okhttp3/internal/cache/DiskLruCache.kt @@ -30,10 +30,12 @@ import okhttp3.internal.closeQuietly import okhttp3.internal.concurrent.Task import okhttp3.internal.concurrent.TaskRunner import okhttp3.internal.io.FileSystem +import okhttp3.internal.isCivilized import okhttp3.internal.okHttpName import okhttp3.internal.platform.Platform import okhttp3.internal.platform.Platform.Companion.WARN import okio.BufferedSink +import okio.ForwardingSource import okio.Sink import okio.Source import okio.blackholeSink @@ -155,6 +157,7 @@ class DiskLruCache internal constructor( internal val lruEntries = LinkedHashMap(0, 0.75f, true) private var redundantOpCount: Int = 0 private var hasJournalErrors: Boolean = false + private var civilizedFileSystem: Boolean = false // Must be read and written when synchronized on 'this'. private var initialized: Boolean = false @@ -225,6 +228,8 @@ class DiskLruCache internal constructor( } } + civilizedFileSystem = fileSystem.isCivilized(journalFileBackup) + // Prefer to pick up where we left off. if (fileSystem.exists(journalFile)) { try { @@ -423,7 +428,6 @@ class DiskLruCache internal constructor( checkNotClosed() validateKey(key) val entry = lruEntries[key] ?: return null - if (!entry.readable) return null val snapshot = entry.snapshot() ?: return null redundantOpCount++ @@ -456,6 +460,10 @@ class DiskLruCache internal constructor( return null // Another edit is in progress. } + if (entry != null && entry.lockingSourceCount != 0) { + return null // We can't write this file because a reader is still reading it. + } + if (mostRecentTrimFailed || mostRecentRebuildFailed) { // The OS has become our enemy! If the trim job failed, it means we are storing more data than // requested by the user. Do not allow edits so we do not go over that limit any further. If @@ -518,7 +526,7 @@ class DiskLruCache internal constructor( for (i in 0 until valueCount) { val dirty = entry.dirtyFiles[i] - if (success) { + if (success && !entry.zombie) { if (fileSystem.exists(dirty)) { val clean = entry.cleanFiles[i] fileSystem.rename(dirty, clean) @@ -532,8 +540,13 @@ class DiskLruCache internal constructor( } } - redundantOpCount++ entry.currentEditor = null + if (entry.zombie) { + removeEntry(entry) + return + } + + redundantOpCount++ journalWriter!!.apply { if (entry.readable || success) { entry.readable = true @@ -588,6 +601,25 @@ class DiskLruCache internal constructor( @Throws(IOException::class) internal fun removeEntry(entry: Entry): Boolean { + // If we can't delete files that are still open, mark this entry as a zombie so its files will + // be deleted when those files are closed. + if (!civilizedFileSystem) { + if (entry.lockingSourceCount > 0) { + // Mark this entry as 'DIRTY' so that if the process crashes this entry won't be used. + journalWriter?.let { + it.writeUtf8(DIRTY) + it.writeByte(' '.toInt()) + it.writeUtf8(entry.key) + it.writeByte('\n'.toInt()) + it.flush() + } + } + if (entry.lockingSourceCount > 0 || entry.currentEditor != null) { + entry.zombie = true + return true + } + } + entry.currentEditor?.detach() // Prevent the edit from completing normally. for (i in 0 until valueCount) { @@ -597,10 +629,12 @@ class DiskLruCache internal constructor( } redundantOpCount++ - journalWriter!!.writeUtf8(REMOVE) - .writeByte(' '.toInt()) - .writeUtf8(entry.key) - .writeByte('\n'.toInt()) + journalWriter?.let { + it.writeUtf8(REMOVE) + it.writeByte(' '.toInt()) + it.writeUtf8(entry.key) + it.writeByte('\n'.toInt()) + } lruEntries.remove(entry.key) if (journalRebuildRequired()) { @@ -637,7 +671,7 @@ class DiskLruCache internal constructor( // Copying for concurrent iteration. for (entry in lruEntries.values.toTypedArray()) { if (entry.currentEditor != null) { - entry.currentEditor!!.abort() + entry.currentEditor?.detach() // Prevent the edit from completing normally. } } @@ -650,12 +684,22 @@ class DiskLruCache internal constructor( @Throws(IOException::class) fun trimToSize() { while (size > maxSize) { - val toEvict = lruEntries.values.iterator().next() - removeEntry(toEvict) + if (!removeOldestEntry()) return } mostRecentTrimFailed = false } + /** Returns true if an entry was removed. This will return false if all entries are zombies. */ + private fun removeOldestEntry(): Boolean { + for (toEvict in lruEntries.values) { + if (!toEvict.zombie) { + removeEntry(toEvict) + return true + } + } + return false + } + /** * Closes the cache and deletes all of its stored values. This will delete all files in the cache * directory including files that weren't created by the cache. @@ -718,12 +762,7 @@ class DiskLruCache internal constructor( if (closed) return false while (delegate.hasNext()) { - val entry = delegate.next() - if (entry == null || !entry.readable) continue // Entry during edit - - val snapshot = entry.snapshot() ?: continue - // Evicted since we copied the entries. - nextSnapshot = snapshot + nextSnapshot = delegate.next()?.snapshot() ?: continue return true } } @@ -795,14 +834,11 @@ class DiskLruCache internal constructor( */ internal fun detach() { if (entry.currentEditor == this) { - for (i in 0 until valueCount) { - try { - fileSystem.delete(entry.dirtyFiles[i]) - } catch (_: IOException) { - // This file is potentially leaked. Not much we can do about that. - } + if (civilizedFileSystem) { + completeEdit(this, false) // Delete it now. + } else { + entry.zombie = true // We can't delete it until the current edit completes. } - entry.currentEditor = null } } @@ -813,7 +849,7 @@ class DiskLruCache internal constructor( fun newSource(index: Int): Source? { synchronized(this@DiskLruCache) { check(!done) - if (!entry.readable || entry.currentEditor != this) { + if (!entry.readable || entry.currentEditor != this || entry.zombie) { return null } return try { @@ -896,9 +932,21 @@ class DiskLruCache internal constructor( /** True if this entry has ever been published. */ internal var readable: Boolean = false - /** The ongoing edit or null if this entry is not being edited. */ + /** True if this entry must be deleted when the current edit or read completes. */ + internal var zombie: Boolean = false + + /** + * The ongoing edit or null if this entry is not being edited. When setting this to null the + * entry must be removed if it is a zombie. + */ internal var currentEditor: Editor? = null + /** + * Sources currently reading this entry before a write or delete can proceed. When decrementing + * this to zero, the entry must be removed if it is a zombie. + */ + internal var lockingSourceCount = 0 + /** The sequence number of the most recently committed edit to this entry. */ internal var sequenceNumber: Long = 0 @@ -940,7 +988,7 @@ class DiskLruCache internal constructor( } @Throws(IOException::class) - private fun invalidLengths(strings: List): IOException { + private fun invalidLengths(strings: List): Nothing { throw IOException("unexpected journal line: $strings") } @@ -952,11 +1000,14 @@ class DiskLruCache internal constructor( internal fun snapshot(): Snapshot? { this@DiskLruCache.assertThreadHoldsLock() + if (!readable) return null + if (!civilizedFileSystem && (currentEditor != null || zombie)) return null + val sources = mutableListOf() val lengths = this.lengths.clone() // Defensive copy since these can be zeroed out. try { for (i in 0 until valueCount) { - sources += fileSystem.source(cleanFiles[i]) + sources += newSource(i) } return Snapshot(key, sequenceNumber, sources, lengths) } catch (_: FileNotFoundException) { @@ -973,6 +1024,28 @@ class DiskLruCache internal constructor( return null } } + + private fun newSource(index: Int): Source { + val fileSource = fileSystem.source(cleanFiles[index]) + if (civilizedFileSystem) return fileSource + + lockingSourceCount++ + return object : ForwardingSource(fileSource) { + var closed = false + override fun close() { + super.close() + if (!closed) { + closed = true + synchronized(this@DiskLruCache) { + lockingSourceCount-- + if (lockingSourceCount == 0 && zombie) { + removeEntry(this@Entry) + } + } + } + } + } + } } companion object { diff --git a/okhttp/src/test/java/okhttp3/internal/cache/DiskLruCacheTest.kt b/okhttp/src/test/java/okhttp3/internal/cache/DiskLruCacheTest.kt index 2017c00e484b..a1a061566980 100644 --- a/okhttp/src/test/java/okhttp3/internal/cache/DiskLruCacheTest.kt +++ b/okhttp/src/test/java/okhttp3/internal/cache/DiskLruCacheTest.kt @@ -16,18 +16,22 @@ package okhttp3.internal.cache import java.io.File +import java.io.FileNotFoundException import java.io.IOException import java.util.ArrayDeque import java.util.NoSuchElementException -import okhttp3.TestUtil.assumeNotWindows +import okhttp3.TestUtil import okhttp3.internal.cache.DiskLruCache.Editor import okhttp3.internal.cache.DiskLruCache.Snapshot import okhttp3.internal.concurrent.TaskFaker import okhttp3.internal.io.FaultyFileSystem import okhttp3.internal.io.FileSystem +import okhttp3.internal.io.InMemoryFileSystem +import okhttp3.internal.io.WindowsFileSystem import okio.Source import okio.buffer import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assumptions.assumeThat import org.junit.After import org.junit.Assert.fail import org.junit.Before @@ -35,15 +39,32 @@ import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder import org.junit.rules.Timeout +import org.junit.runner.RunWith +import org.junit.runners.Parameterized +import org.junit.runners.Parameterized.Parameters + +@RunWith(Parameterized::class) +class DiskLruCacheTest( + baseFileSystem: FileSystem, + private val windows: Boolean +) { + private var fileSystem = FaultyFileSystem(baseFileSystem) -class DiskLruCacheTest { @Rule @JvmField val tempDir = TemporaryFolder() @Rule @JvmField val timeout = Timeout(60 * 1000) - private val fileSystem = FaultyFileSystem(FileSystem.SYSTEM) + companion object { + @Parameters(name = "{0}") @JvmStatic + fun parameters(): Collection> = listOf( + arrayOf(FileSystem.SYSTEM, TestUtil.windows), + arrayOf(WindowsFileSystem(InMemoryFileSystem()), true), + arrayOf(InMemoryFileSystem(), false) + ) + } + private val appVersion = 100 private lateinit var cacheDir: File private lateinit var journalFile: File @@ -65,6 +86,7 @@ class DiskLruCacheTest { @Before fun setUp() { cacheDir = tempDir.root + fileSystem.deleteContents(cacheDir) journalFile = File(cacheDir, DiskLruCache.JOURNAL_FILE) journalBkpFile = File(cacheDir, DiskLruCache.JOURNAL_FILE_BACKUP) createNewCache() @@ -227,10 +249,16 @@ class DiskLruCacheTest { assertJournalEquals("DIRTY k1", "REMOVE k1") } - @Test fun unterminatedEditIsRevertedOnClose() { - cache.edit("k1") + /** On Windows we have to wait until the edit is committed before we can delete its files. */ + @Test fun `unterminated edit is reverted on cache close`() { + val editor = cache.edit("k1")!! + editor.setString(0, "AB") + editor.setString(1, "C") cache.close() - assertJournalEquals("DIRTY k1", "REMOVE k1") + val expected = if (windows) arrayOf("DIRTY k1") else arrayOf("DIRTY k1", "REMOVE k1") + assertJournalEquals(*expected) + editor.commit() + assertJournalEquals(*expected) // 'REMOVE k1' not written because journal is closed. } @Test fun journalDoesNotIncludeReadOfYetUnpublishedValue() { @@ -300,7 +328,7 @@ class DiskLruCacheTest { * the same key can see different data. */ @Test fun readAndWriteOverlapsMaintainConsistency() { - assumeNotWindows() + assumeThat(windows).isFalse() // Can't edit while a read is in progress. val v1Creator = cache.edit("k1")!! v1Creator.setString(0, "AAaa") @@ -338,7 +366,7 @@ class DiskLruCacheTest { writeFile(cleanFile1, "B") writeFile(dirtyFile0, "C") writeFile(dirtyFile1, "D") - createJournal("CLEAN k1 1 1", "DIRTY k1") + createJournal("CLEAN k1 1 1", "DIRTY k1") createNewCache() assertThat(fileSystem.exists(cleanFile0)).isFalse() assertThat(fileSystem.exists(cleanFile1)).isFalse() @@ -946,9 +974,9 @@ class DiskLruCacheTest { } @Test fun editSameVersion() { - assumeNotWindows() set("a", "a", "a") val snapshot = cache["a"]!! + snapshot.close() val editor = snapshot.edit()!! editor.setString(1, "a2") editor.commit() @@ -956,9 +984,9 @@ class DiskLruCacheTest { } @Test fun editSnapshotAfterChangeAborted() { - assumeNotWindows() set("a", "a", "a") val snapshot = cache["a"]!! + snapshot.close() val toAbort = snapshot.edit()!! toAbort.setString(0, "b") toAbort.abort() @@ -969,9 +997,9 @@ class DiskLruCacheTest { } @Test fun editSnapshotAfterChangeCommitted() { - assumeNotWindows() set("a", "a", "a") val snapshot = cache["a"]!! + snapshot.close() val toAbort = snapshot.edit()!! toAbort.setString(0, "b") toAbort.commit() @@ -979,7 +1007,6 @@ class DiskLruCacheTest { } @Test fun editSinceEvicted() { - assumeNotWindows() cache.close() createNewCacheWithSize(10) set("a", "aa", "aaa") // size 5 @@ -991,11 +1018,11 @@ class DiskLruCacheTest { } @Test fun editSinceEvictedAndRecreated() { - assumeNotWindows() cache.close() createNewCacheWithSize(10) set("a", "aa", "aaa") // size 5 val snapshot = cache["a"]!! + snapshot.close() set("b", "bb", "bbb") // size 5 set("c", "cc", "ccc") // size 5; will evict 'A' set("a", "a", "aaaa") // size 5; will evict 'B' @@ -1005,7 +1032,8 @@ class DiskLruCacheTest { /** @see [Issue 2](https://github.com/JakeWharton/DiskLruCache/issues/2) */ @Test fun aggressiveClearingHandlesWrite() { - assumeNotWindows() + assumeThat(windows).isFalse() // Can't deleteContents while the journal is open. + fileSystem.deleteContents(tempDir.root) set("a", "a", "a") assertValue("a", "a", "a") @@ -1013,9 +1041,10 @@ class DiskLruCacheTest { /** @see [Issue 2](https://github.com/JakeWharton/DiskLruCache/issues/2) */ @Test fun aggressiveClearingHandlesEdit() { - assumeNotWindows() + assumeThat(windows).isFalse() // Can't deleteContents while the journal is open. + set("a", "a", "a") - val a = cache["a"]!!.edit()!! + val a = cache.edit("a")!! fileSystem.deleteContents(tempDir.root) a.setString(1, "a2") a.commit() @@ -1029,10 +1058,11 @@ class DiskLruCacheTest { /** @see [Issue 2](https://github.com/JakeWharton/DiskLruCache/issues/2) */ @Test fun aggressiveClearingHandlesPartialEdit() { - assumeNotWindows() + assumeThat(windows).isFalse() // Can't deleteContents while the journal is open. + set("a", "a", "a") set("b", "b", "b") - val a = cache["a"]!!.edit()!! + val a = cache.edit("a")!! a.setString(0, "a1") fileSystem.deleteContents(tempDir.root) a.setString(1, "a2") @@ -1042,7 +1072,8 @@ class DiskLruCacheTest { /** @see [Issue 2](https://github.com/JakeWharton/DiskLruCache/issues/2) */ @Test fun aggressiveClearingHandlesRead() { - assumeNotWindows() + assumeThat(windows).isFalse() // Can't deleteContents while the journal is open. + fileSystem.deleteContents(tempDir.root) assertThat(cache["a"]).isNull() } @@ -1052,13 +1083,17 @@ class DiskLruCacheTest { * being edited required deletion for the operation to complete. */ @Test fun trimToSizeWithActiveEdit() { + val expectedByteCount = if (windows) 10L else 0L + val afterRemoveFileContents = if (windows) "a1234" else null + set("a", "a1234", "a1234") val a = cache.edit("a")!! a.setString(0, "a123") cache.maxSize = 8 // Smaller than the sum of active edits! cache.flush() // Force trimToSize(). - assertThat(cache.size()).isEqualTo(0) - assertThat(cache["a"]).isNull() + assertThat(cache.size()).isEqualTo(expectedByteCount) + assertThat(readFileOrNull(getCleanFile("a", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getCleanFile("a", 1))).isEqualTo(afterRemoveFileContents) // After the edit is completed, its entry is still gone. a.setString(1, "a1") @@ -1087,37 +1122,47 @@ class DiskLruCacheTest { } @Test fun evictAllWithPartialEditDoesNotStoreAValue() { + val expectedByteCount = if (windows) 2L else 0L + set("a", "a", "a") val a = cache.edit("a")!! a.setString(0, "a1") a.setString(1, "a2") cache.evictAll() - assertThat(cache.size()).isEqualTo(0) + assertThat(cache.size()).isEqualTo(expectedByteCount) a.commit() assertAbsent("a") } @Test fun evictAllDoesntInterruptPartialRead() { - assumeNotWindows() + val expectedByteCount = if (windows) 2L else 0L + val afterRemoveFileContents = if (windows) "a" else null + set("a", "a", "a") cache["a"]!!.use { it.assertValue(0, "a") cache.evictAll() - assertThat(cache.size()).isEqualTo(0) - assertAbsent("a") + assertThat(cache.size()).isEqualTo(expectedByteCount) + assertThat(readFileOrNull(getCleanFile("a", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getCleanFile("a", 1))).isEqualTo(afterRemoveFileContents) it.assertValue(1, "a") } + assertThat(cache.size()).isEqualTo(0L) } @Test fun editSnapshotAfterEvictAllReturnsNullDueToStaleValue() { - assumeNotWindows() + val expectedByteCount = if (windows) 2L else 0L + val afterRemoveFileContents = if (windows) "a" else null + set("a", "a", "a") cache["a"]!!.use { cache.evictAll() - assertThat(cache.size()).isEqualTo(0) - assertAbsent("a") + assertThat(cache.size()).isEqualTo(expectedByteCount) + assertThat(readFileOrNull(getCleanFile("a", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getCleanFile("a", 1))).isEqualTo(afterRemoveFileContents) assertThat(it.edit()).isNull() } + assertThat(cache.size()).isEqualTo(0L) } @Test operator fun iterator() { @@ -1514,6 +1559,8 @@ class DiskLruCacheTest { } @Test fun noSizeCorruptionAfterCreatorDetached() { + assumeThat(windows).isFalse() // Windows can't have two concurrent editors. + // Create an editor for k1. Detach it by clearing the cache. val editor = cache.edit("k1")!! editor.setString(0, "a") @@ -1531,6 +1578,8 @@ class DiskLruCacheTest { } @Test fun noSizeCorruptionAfterEditorDetached() { + assumeThat(windows).isFalse() // Windows can't have two concurrent editors. + set("k1", "a", "a") // Create an editor for k1. Detach it by clearing the cache. @@ -1556,8 +1605,23 @@ class DiskLruCacheTest { assertThat(editor.newSource(0)).isNull() } - @Test fun editsDiscardedAfterEditorDetached() { - assumeNotWindows() + @Test fun `edit discarded after editor detached`() { + set("k1", "a", "a") + + // Create an editor, then detach it. + val editor = cache.edit("k1")!! + editor.newSink(0).buffer().use { sink -> + cache.evictAll() + + // Complete the original edit. It goes into a black hole. + sink.writeUtf8("bb") + } + assertThat(cache["k1"]).isNull() + } + + @Test fun `edit discarded after editor detached with concurrent write`() { + assumeThat(windows).isFalse() // Windows can't have two concurrent editors. + set("k1", "a", "a") // Create an editor, then detach it. @@ -1596,6 +1660,167 @@ class DiskLruCacheTest { assertThat(snapshotAfterCommit.hasNext()).withFailMessage("Entry has been removed during creation.").isTrue() } + @Test fun `Windows cannot read while writing`() { + assumeThat(windows).isTrue() + + set("k1", "a", "a") + val editor = cache.edit("k1")!! + assertThat(cache["k1"]).isNull() + editor.commit() + } + + @Test fun `Windows cannot write while reading`() { + assumeThat(windows).isTrue() + + set("k1", "a", "a") + val snapshot = cache["k1"]!! + assertThat(cache.edit("k1")).isNull() + snapshot.close() + } + + @Test fun `can read while reading`() { + set("k1", "a", "a") + cache["k1"]!!.use { snapshot1 -> + snapshot1.assertValue(0, "a") + cache["k1"]!!.use { snapshot2 -> + snapshot2.assertValue(0, "a") + snapshot1.assertValue(1, "a") + snapshot2.assertValue(1, "a") + } + } + } + + @Test fun `remove while reading creates zombie that is removed when read finishes`() { + val afterRemoveFileContents = if (windows) "a" else null + + set("k1", "a", "a") + cache["k1"]!!.use { snapshot1 -> + cache.remove("k1") + + // On Windows files still exist with open with 2 open sources. + assertThat(readFileOrNull(getCleanFile("k1", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + + // On Windows files still exist with open with 1 open source. + snapshot1.assertValue(0, "a") + assertThat(readFileOrNull(getCleanFile("k1", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + + // On all platforms files are deleted when all sources are closed. + snapshot1.assertValue(1, "a") + assertThat(readFileOrNull(getCleanFile("k1", 0))).isNull() + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + } + } + + @Test fun `remove while writing creates zombie that is removed when write finishes`() { + val afterRemoveFileContents = if (windows) "a" else null + + set("k1", "a", "a") + val editor = cache.edit("k1")!! + cache.remove("k1") + assertThat(cache["k1"]).isNull() + + // On Windows files still exist while being edited. + assertThat(readFileOrNull(getCleanFile("k1", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + + // On all platforms files are deleted when the edit completes. + editor.commit() + assertThat(readFileOrNull(getCleanFile("k1", 0))).isNull() + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + } + + @Test fun `Windows cannot read zombie entry`() { + assumeThat(windows).isTrue() + + set("k1", "a", "a") + cache["k1"]!!.use { + cache.remove("k1") + assertThat(cache["k1"]).isNull() + } + } + + @Test fun `Windows cannot write zombie entry`() { + assumeThat(windows).isTrue() + + set("k1", "a", "a") + cache["k1"]!!.use { + cache.remove("k1") + assertThat(cache.edit("k1")).isNull() + } + } + + @Test fun `removed entry absent when iterating`() { + set("k1", "a", "a") + cache["k1"]!!.use { + cache.remove("k1") + val snapshots = cache.snapshots() + assertThat(snapshots.hasNext()).isFalse() + } + } + + @Test fun `close with zombie read`() { + val afterRemoveFileContents = if (windows) "a" else null + + set("k1", "a", "a") + cache["k1"]!!.use { + cache.remove("k1") + + // After we close the cache the files continue to exist! + cache.close() + assertThat(readFileOrNull(getCleanFile("k1", 0))).isEqualTo(afterRemoveFileContents) + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + + // But they disappear when the sources are closed. + it.assertValue(0, "a") + it.assertValue(1, "a") + assertThat(readFileOrNull(getCleanFile("k1", 0))).isNull() + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + } + } + + @Test fun `close with zombie write`() { + val afterRemoveCleanFileContents = if (windows) "a" else null + val afterRemoveDirtyFileContents = if (windows) "" else null + + set("k1", "a", "a") + val editor = cache.edit("k1")!! + val sink0 = editor.newSink(0) + cache.remove("k1") + + // After we close the cache the files continue to exist! + cache.close() + assertThat(readFileOrNull(getCleanFile("k1", 0))).isEqualTo(afterRemoveCleanFileContents) + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isEqualTo(afterRemoveDirtyFileContents) + + // But they disappear when the edit completes. + sink0.close() + editor.commit() + assertThat(readFileOrNull(getCleanFile("k1", 0))).isNull() + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + } + + @Test fun `close with completed zombie write`() { + val afterRemoveCleanFileContents = if (windows) "a" else null + val afterRemoveDirtyFileContents = if (windows) "b" else null + + set("k1", "a", "a") + val editor = cache.edit("k1")!! + editor.setString(0, "b") + cache.remove("k1") + + // After we close the cache the files continue to exist! + cache.close() + assertThat(readFileOrNull(getCleanFile("k1", 0))).isEqualTo(afterRemoveCleanFileContents) + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isEqualTo(afterRemoveDirtyFileContents) + + // But they disappear when the edit completes. + editor.commit() + assertThat(readFileOrNull(getCleanFile("k1", 0))).isNull() + assertThat(readFileOrNull(getDirtyFile("k1", 0))).isNull() + } + private fun assertJournalEquals(vararg expectedBodyLines: String) { assertThat(readJournalLines()).isEqualTo( listOf(DiskLruCache.MAGIC, DiskLruCache.VERSION_1, "100", "2", "") + expectedBodyLines) @@ -1653,6 +1878,16 @@ class DiskLruCacheTest { } } + private fun readFileOrNull(file: File): String? { + try { + fileSystem.source(file).buffer().use { + return it.readUtf8() + } + } catch (_: FileNotFoundException) { + return null + } + } + fun writeFile(file: File, content: String) { fileSystem.sink(file).buffer().use { sink -> sink.writeUtf8(content) @@ -1709,8 +1944,10 @@ class DiskLruCacheTest { } private fun Snapshot.assertValue(index: Int, value: String) { - assertThat(sourceAsString(getSource(index))).isEqualTo(value) - assertThat(getLength(index)).isEqualTo(value.length.toLong()) + getSource(index).use { source -> + assertThat(sourceAsString(source)).isEqualTo(value) + assertThat(getLength(index)).isEqualTo(value.length.toLong()) + } } private fun sourceAsString(source: Source) = source.buffer().readUtf8() diff --git a/okhttp/src/test/java/okhttp3/internal/io/FaultyFileSystem.java b/okhttp/src/test/java/okhttp3/internal/io/FaultyFileSystem.java index bc0263d830dd..4e3b9f33daa6 100644 --- a/okhttp/src/test/java/okhttp3/internal/io/FaultyFileSystem.java +++ b/okhttp/src/test/java/okhttp3/internal/io/FaultyFileSystem.java @@ -107,4 +107,8 @@ public FaultySink(Sink delegate, File file) { super.write(source, byteCount); } } + + @Override public String toString() { + return "Faulty " + delegate; + } }