Fix DiskLruCache to work on Windows

As originally designed DiskLruCache assumes an inode-like
file system, where it's fine to delete files that are
currently being read or written.

On Windows the file system forbids this, so we must be
more careful when deleting and renaming files. These
operations come up a lot internally in the cache:
 - deleting to evict an entry
 - renaming to commit a dirty file to a clean file

The workaround is simple if unsatisfying: we don't
permit concurrent reads and writes on Windows. We
can have multiple concurrent reders, or a single
writer.

One challenge in this implementation is detecting
whether we're running on Windows or a good operating
system. We deliberately don't look at System properties
here because the OS and file system may disagree, such
as when a Windows machine has an ext4 partition, or when
a Linux machine has an NTFS partition. Instead of detecting
we just attempt an edit and see what happens.

Another challenge in this implementation is what to
do when a file needs to be deleted but cannot be because
it is currently open. In such cases we now mark the
cache entry as a 'zombie'. When the files are later
closed they now check for zombie status and delete the
files if necessary. Note that it is not possible to
store a new cache entry while it is a zombie.

Closes: https://github.com/square/okhttp/issues/5761

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/

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<File, Buffer> = mutableMapOf()
+  private val files = mutableMapOf<File, Buffer>()
-  private val openSources: MutableMap<Source, File> = IdentityHashMap()
+  private val openSources = IdentityHashMap<Source, File>()
-  private val openSinks: MutableMap<Sink, File> = IdentityHashMap()
+  private val openSinks = IdentityHashMap<Sink, File>()
 
-  override fun apply(
+  override fun apply(base: Statement, description: Description): Statement {
-    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<String> = mutableListOf()
+    val openResources = mutableListOf<String>()
     for (file in openSources.values) {
       openResources.add("Source for $file")
     }
     for (file in openSinks.values) {
       openResources.add("Sink for $file")
     }
-    if (!openResources.isEmpty()) {
+    check(openResources.isEmpty()) {
-      val builder =
+      "Resources acquired but not closed:\n * ${openResources.joinToString(separator = "\n * ")}"
-        StringBuilder("Resources acquired but not closed:")
-      for (resource in openResources) {
-        builder.append("\n * ")
-            .append(resource)
-      }
-      throw IllegalStateException(builder.toString())
     }
   }
 
-  @Throws(
+  @Throws(FileNotFoundException::class)
-      FileNotFoundException::class
+  override fun source(file: File): Source {
-  ) 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 {
+  override fun sink(file: File) = sink(file, false)
-    return sink(file, false)
-  }
 
-  @Throws(
+  @Throws(FileNotFoundException::class)
-      FileNotFoundException::class
+  override fun appendingSink(file: File) = sink(file, true)
-  ) override fun appendingSink(file: File): Sink {
-    return sink(file, true)
-  }
 
-  private fun sink(
+  private fun sink(file: File, appending: Boolean): 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 {
+  override fun exists(file: File) = files.containsKey(file)
-    return files.containsKey(file)
+
+  override fun size(file: File) = files[file]?.size ?: 0L
+
+  @Throws(IOException::class)
+  override fun rename(from: File, to: File) {
+    files[to] = files.remove(from) ?: throw FileNotFoundException()
   }
 
-  override fun size(file: File): Long {
+  @Throws(IOException::class)
-    val buffer = files[file]
+  override fun deleteContents(directory: File) {
-    return buffer?.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 deleteContents(directory: File) {
-    val prefix = "$directory/"
     val i = files.keys.iterator()
     while (i.hasNext()) {
       val file = i.next()
-      if (file.toString()
+      if (file.isDescendentOf(directory)) i.remove()
-              .startsWith(prefix)
-      ) i.remove()
     }
   }
 

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
 

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)

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<String, Entry>(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)
+    journalWriter?.let {
-        .writeByte(' '.toInt())
+      it.writeUtf8(REMOVE)
-        .writeUtf8(entry.key)
+      it.writeByte(' '.toInt())
-        .writeByte('\n'.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()
+      if (!removeOldestEntry()) return
-      removeEntry(toEvict)
     }
     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()
+            nextSnapshot = delegate.next()?.snapshot() ?: continue
-            if (entry == null || !entry.readable) continue // Entry during edit
-
-            val snapshot = entry.snapshot() ?: continue
-            // Evicted since we copied the entries.
-            nextSnapshot = snapshot
             return true
           }
         }
@@ -795,15 +834,12 @@ class DiskLruCache internal constructor(
      */
     internal fun detach() {
       if (entry.currentEditor == this) {
-        for (i in 0 until valueCount) {
+        if (civilizedFileSystem) {
-          try {
+          completeEdit(this, false) // Delete it now.
-            fileSystem.delete(entry.dirtyFiles[i])
+        } else {
-          } catch (_: IOException) {
+          entry.zombie = true // We can't delete it until the current edit completes.
-            // This file is potentially leaked. Not much we can do about that.
         }
       }
-        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<String>): IOException {
+    private fun invalidLengths(strings: List<String>): 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<Source>()
       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 {

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<Array<Any>> = 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() {
+  /** On Windows we have to wait until the edit is committed before we can delete its files. */
-    cache.edit("k1")
+  @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")
@@ -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.size()).isEqualTo(expectedByteCount)
-    assertThat(cache["a"]).isNull()
+    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)
+      assertThat(cache.size()).isEqualTo(expectedByteCount)
-      assertAbsent("a")
+      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)
+      assertThat(cache.size()).isEqualTo(expectedByteCount)
-      assertAbsent("a")
+      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() {
+  @Test fun `edit discarded after editor detached`() {
-    assumeNotWindows()
+    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,9 +1944,11 @@ class DiskLruCacheTest {
   }
 
   private fun Snapshot.assertValue(index: Int, value: String) {
-    assertThat(sourceAsString(getSource(index))).isEqualTo(value)
+    getSource(index).use { source ->
+      assertThat(sourceAsString(source)).isEqualTo(value)
       assertThat(getLength(index)).isEqualTo(value.length.toLong())
     }
+  }
 
   private fun sourceAsString(source: Source) = source.buffer().readUtf8()
 

okhttp/src/test/java/okhttp3/internal/io/FaultyFileSystem.java

@@ -107,4 +107,8 @@ public final class FaultyFileSystem implements FileSystem {
       super.write(source, byteCount);
     }
   }
+
+  @Override public String toString() {
+    return "Faulty " + delegate;
+  }
 }