diff --git a/android-test/src/androidTest/java/okhttp/android/test/OkHttpTest.kt b/android-test/src/androidTest/java/okhttp/android/test/OkHttpTest.kt index 1064756cb74d..90e22873eee0 100644 --- a/android-test/src/androidTest/java/okhttp/android/test/OkHttpTest.kt +++ b/android-test/src/androidTest/java/okhttp/android/test/OkHttpTest.kt @@ -865,6 +865,7 @@ class OkHttpTest { assertEquals(setOf(OkHttpTest::class.java.name), testHandler.calls.keys) } + @Test fun testCachedRequest() { enableTls() diff --git a/okcurl/src/test/kotlin/okhttp3/curl/MainTest.kt b/okcurl/src/test/kotlin/okhttp3/curl/MainTest.kt index ae31c41c2521..3af7d5de0ac6 100644 --- a/okcurl/src/test/kotlin/okhttp3/curl/MainTest.kt +++ b/okcurl/src/test/kotlin/okhttp3/curl/MainTest.kt @@ -23,7 +23,9 @@ import com.github.ajalt.clikt.core.parse import java.io.IOException import kotlin.test.Test import okhttp3.RequestBody +import okhttp3.TestUtil import okio.Buffer +import org.junit.jupiter.api.BeforeAll class MainTest { @Test @@ -138,5 +140,12 @@ class MainTest { throw RuntimeException(e) } } + + @JvmStatic + @BeforeAll + fun beforeAll() { +// https://github.com/square/okhttp/issues/8342 + TestUtil.assumeNotWindows() + } } } diff --git a/okcurl/src/test/kotlin/okhttp3/curl/OkcurlTest.kt b/okcurl/src/test/kotlin/okhttp3/curl/OkcurlTest.kt index f7a68dbcbc0a..0e4d4f5b4888 100644 --- a/okcurl/src/test/kotlin/okhttp3/curl/OkcurlTest.kt +++ b/okcurl/src/test/kotlin/okhttp3/curl/OkcurlTest.kt @@ -17,10 +17,21 @@ package okhttp3.curl import com.github.ajalt.clikt.core.main import kotlin.test.Test +import okhttp3.TestUtil +import org.junit.jupiter.api.BeforeAll class OkcurlTest { @Test fun simple() { Main().main(listOf("--help")) } + + companion object { + @JvmStatic + @BeforeAll + fun beforeAll() { +// https://github.com/square/okhttp/issues/8342 + TestUtil.assumeNotWindows() + } + } } diff --git a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt index 76f1a3b5392b..76218fd400a2 100644 --- a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt +++ b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt @@ -176,7 +176,7 @@ class DnsOverHttpsTest { // 5. unsuccessful response @Test fun usesCache() { - val cache = Cache(cacheFs, "cache".toPath(), (100 * 1024).toLong()) + val cache = Cache(cacheFs, "cache-usesCache".toPath(), (100 * 1024).toLong()) val cachedClient = bootstrapClient.newBuilder().cache(cache).build() val cachedDns = buildLocalhost(cachedClient, false) @@ -220,7 +220,7 @@ class DnsOverHttpsTest { @Test fun usesCacheEvenForPost() { - val cache = Cache(cacheFs, "cache".toPath(), (100 * 1024).toLong()) + val cache = Cache(cacheFs, "cache-usesCacheEvenForPost".toPath(), (100 * 1024).toLong()) val cachedClient = bootstrapClient.newBuilder().cache(cache).build() val cachedDns = buildLocalhost(cachedClient, false, post = true) repeat(2) { @@ -259,6 +259,8 @@ class DnsOverHttpsTest { .isEqualTo("/lookup?ct") assertThat(cacheEvents()).containsExactly("CacheMiss") + + cache.close() } @Test @@ -305,6 +307,8 @@ class DnsOverHttpsTest { .isEqualTo("/lookup?ct&dns=AAABAAABAAAAAAAABmdvb2dsZQNjb20AAAEAAQ") assertThat(cacheEvents()).containsExactly("CacheMiss") + + cache.close() } private fun cacheEvents(): List { diff --git a/okhttp/build.gradle.kts b/okhttp/build.gradle.kts index 7a562870cf62..68b69b27aea7 100644 --- a/okhttp/build.gradle.kts +++ b/okhttp/build.gradle.kts @@ -147,6 +147,7 @@ kotlin { implementation(libs.kotlin.test.junit) implementation(libs.openjsse) compileOnly(libs.findbugs.jsr305) + implementation(libs.kotlinx.coroutines.test) implementation(libs.junit.jupiter.engine) implementation(libs.junit.vintage.engine) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 69d79cd9394f..f5489b9bca08 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -150,6 +150,7 @@ class Cache internal constructor( maxSize: Long, fileSystem: FileSystem, taskRunner: TaskRunner, + useCacheLock: Boolean, ) : Closeable, Flushable { /** Create a cache of at most [maxSize] bytes in [directory]. */ constructor( @@ -157,10 +158,11 @@ class Cache internal constructor( directory: Path, maxSize: Long, ) : this( - directory, - maxSize, - fileSystem, - TaskRunner.INSTANCE, + directory = directory, + maxSize = maxSize, + fileSystem = fileSystem, + taskRunner = TaskRunner.INSTANCE, + useCacheLock = true, ) /** Create a cache of at most [maxSize] bytes in [directory]. */ @@ -178,6 +180,7 @@ class Cache internal constructor( valueCount = ENTRY_COUNT, maxSize = maxSize, taskRunner = taskRunner, + useCacheLock = useCacheLock, ) // read and write statistics, all guarded by 'this'. diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheLock.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheLock.kt new file mode 100644 index 000000000000..fbfb63f4adfd --- /dev/null +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheLock.kt @@ -0,0 +1,115 @@ +/* + * Copyright (C) 2024 Block, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package okhttp3.internal.cache + +import java.io.Closeable +import java.io.File +import java.nio.channels.FileChannel +import java.nio.file.StandardOpenOption +import java.util.Collections +import okio.FileSystem +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()) + + /** + * 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, + ): Closeable { + val memoryLock = inMemoryLock(directory) + + // check if possibly a non System file system + if (FileSystem.SYSTEM.exists(directory)) { + try { + val fileSystemLock = fileSystemLock(directory) + + return Closeable { + memoryLock.close() + fileSystemLock.close() + } + } catch (le: LockException) { + if (fileSystemSupportsLock(fileSystem)) { + memoryLock.close() + throw le + } + } + } + + return memoryLock + } + + @IgnoreJRERequirement // Not called on legacy Android + internal fun fileSystemSupportsLock(fileSystem: FileSystem): Boolean { + val tmpLockFile = File.createTempFile("test-", ".lock") + + if (!fileSystem.exists(tmpLockFile.toOkioPath())) { + return false + } + + val channel = FileChannel.open(tmpLockFile.toPath(), StandardOpenOption.APPEND) + + return channel.tryLock().apply { close() } != null + } + + /** + * Create an in-memory lock, avoiding two open Cache instances. + */ + @IgnoreJRERequirement // D8 supports put if absent + 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) + } + return okio.Closeable { + openCaches.remove(directory) + } + } + + /** + * Create a file system lock, that excludes other processes. However within the process a + * memory lock is also needed, since locks don't work within a single process. + */ + @IgnoreJRERequirement // Not called on legacy Android + internal fun fileSystemLock(directory: Path): Closeable { + // update once https://github.com/square/okio/issues/1464 is available + + val lockFile = directory / "lock" + lockFile.toFile().createNewFile() + val channel = FileChannel.open(lockFile.toNioPath(), StandardOpenOption.APPEND) + + channel.tryLock() ?: throw LockException("Cache already open at '$directory' in another process") + + return okio.Closeable { + channel.close() + } + } +} + +class LockException(message: String, cause: Exception? = null) : Exception(message, cause) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/DiskLruCache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/DiskLruCache.kt index e581cd068b90..a5d374d1a891 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/DiskLruCache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/DiskLruCache.kt @@ -20,6 +20,7 @@ import java.io.EOFException import java.io.Flushable import java.io.IOException import okhttp3.internal.assertThreadHoldsLock +import okhttp3.internal.cache.CacheLock.openLock import okhttp3.internal.cache.DiskLruCache.Editor import okhttp3.internal.closeQuietly import okhttp3.internal.concurrent.Task @@ -94,7 +95,10 @@ class DiskLruCache( maxSize: Long, /** Used for asynchronous journal rebuilds. */ taskRunner: TaskRunner, + private val useCacheLock: Boolean, ) : Closeable, Flushable { + lateinit var cacheLock: Closeable + internal val fileSystem: FileSystem = object : ForwardingFileSystem(fileSystem) { override fun sink( @@ -242,33 +246,42 @@ class DiskLruCache( civilizedFileSystem = fileSystem.isCivilized(journalFileBackup) - // Prefer to pick up where we left off. - if (fileSystem.exists(journalFile)) { - try { - readJournal() - processJournal() - initialized = true - return - } catch (journalIsCorrupt: IOException) { - Platform.get().log( - "DiskLruCache $directory is corrupt: ${journalIsCorrupt.message}, removing", - WARN, - journalIsCorrupt, - ) - } + cacheLock = if (useCacheLock) openLock(fileSystem, directory) else Closeable {} - // The cache is corrupted, attempt to delete the contents of the directory. This can throw and - // we'll let that propagate out as it likely means there is a severe filesystem problem. - try { - delete() - } finally { - closed = false + try { + // Prefer to pick up where we left off. + if (fileSystem.exists(journalFile)) { + try { + readJournal() + processJournal() + initialized = true + return + } catch (journalIsCorrupt: IOException) { + Platform.get().log( + "DiskLruCache $directory is corrupt: ${journalIsCorrupt.message}, removing", + WARN, + journalIsCorrupt, + ) + } + + // The cache is corrupted, attempt to delete the contents of the directory. This can throw and + // we'll let that propagate out as it likely means there is a severe filesystem problem. + try { + delete() + } finally { + closed = false + } } - } - rebuildJournal() + rebuildJournal() - initialized = true + initialized = true + } finally { + // If anything failed, leave without a cache lock open + if (!initialized) { + cacheLock.close() + } + } } @Throws(IOException::class) @@ -705,6 +718,8 @@ class DiskLruCache( return } + cacheLock.close() + // Copying for concurrent iteration. for (entry in lruEntries.values.toTypedArray()) { if (entry.currentEditor != null) { diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache2/Relay.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache2/Relay.kt index 6ae6e61809a1..7ae877622f3f 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache2/Relay.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache2/Relay.kt @@ -344,7 +344,10 @@ class Relay private constructor( val header = Buffer() fileOperator.read(0, header, FILE_HEADER_SIZE) val prefix = header.readByteString(PREFIX_CLEAN.size.toLong()) - if (prefix != PREFIX_CLEAN) throw IOException("unreadable cache file") + if (prefix != PREFIX_CLEAN) { + randomAccessFile.close() + throw IOException("unreadable cache file") + } val upstreamSize = header.readLong() val metadataSize = header.readLong() diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheLockTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheLockTest.kt new file mode 100644 index 000000000000..cb50d051b370 --- /dev/null +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheLockTest.kt @@ -0,0 +1,176 @@ +/* + * Copyright (C) 2024 Block, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package okhttp3 + +import assertk.assertThat +import assertk.assertions.isEqualTo +import java.nio.file.Path +import java.util.Optional +import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import okhttp3.internal.cache.LockException +import okhttp3.testing.PlatformRule +import okhttp3.testing.PlatformVersion +import okio.Closeable +import okio.FileSystem +import okio.Path.Companion.toOkioPath +import okio.Path.Companion.toPath +import okio.fakefilesystem.FakeFileSystem +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assumptions.assumeTrue +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows +import org.junit.jupiter.api.extension.RegisterExtension +import org.junit.jupiter.api.io.TempDir + +@org.junit.jupiter.api.parallel.Isolated +class CacheLockTest { + @JvmField + @RegisterExtension + val platform = PlatformRule() + + private lateinit var tempDir: okio.Path + private val toClose = mutableListOf() + + @BeforeEach + fun setup( + @TempDir tempDir: Path, + ) { + this.tempDir = tempDir.toOkioPath() + } + + @AfterEach + fun cleanup() { + toClose.forEach { + it.close() + } + } + + @Test + fun testCacheLock() { + openCache(tempDir) + + val lockException = + assertThrows { + openCache(tempDir) + } + assertThat(lockException.message).isEqualTo("Cache already open at '$tempDir' in same process") + } + + @Test + fun testCacheLockAfterClose() { + val cache1 = openCache(tempDir) + + cache1.close() + + openCache(tempDir) + } + + @Test + fun testCacheLockDifferentPath() { + openCache(tempDir / "a") + + openCache(tempDir / "b") + } + + @Test + fun testCacheLockDifferentProcess() { + runBlocking { + // No java command to execute LockTestProgram.java + platform.assumeNotAndroid() + assumeTrue(PlatformVersion.majorVersion >= 11) + + val lockFile = tempDir / "lock" + lockFile.toFile().createNewFile() + + val javaExe = + if (PlatformVersion.majorVersion >= 9) { + val info = + Class.forName("java.lang.ProcessHandle").run { + val handle = getMethod("current").invoke(null) + getMethod("info").invoke(handle) + } + val command = Class.forName("java.lang.ProcessHandle\$Info").getMethod("command").invoke(info) as Optional<*> + (command.get() as String).toPath() + } else { + System.getenv("JAVA_HOME").toPath() / "bin/java" + } + + val process = + ProcessBuilder().command( + javaExe.toString(), + "src/jvmTest/kotlin/okhttp3/LockTestProgram.java", + (lockFile.toString()), + ) + .redirectErrorStream(true) + .start() + + val output = process.inputStream.bufferedReader() + + try { + withTimeout(5.seconds) { + assertThat(output.readLine()).isEqualTo("Locking $lockFile") + assertThat(output.readLine()).isEqualTo("Locked $lockFile") + } + + val lockException = + assertThrows { + openCache(tempDir) + } + assertThat(lockException.message).isEqualTo("Cache already open at '$tempDir' in another process") + } finally { + process.destroy() + } + + delay(100) + + // Should work again once process is killed + openCache(tempDir) + } + } + + @Test + fun testCacheLockOnFakeFileSystem() { + val fileSystem = FakeFileSystem() + val fakeCacheDir = "/missing".toPath() + + val cache = openCache(fakeCacheDir, fileSystem) + + val lockException = + assertThrows { + 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(fileSystem, directory, 10_000).apply { + // force early LRU initialisation + initialize() + toClose.add(this) + } + } +} diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 1400aadaeb68..9d36373a359f 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -81,6 +81,7 @@ class CacheTest { private lateinit var client: OkHttpClient private lateinit var cache: Cache private val cookieManager = CookieManager() + private val cachePath = "/cache-CacheTest".toPath() @BeforeEach fun setUp( @@ -92,7 +93,7 @@ class CacheTest { platform.assumeNotOpenJSSE() server.protocolNegotiationEnabled = false fileSystem.emulateUnix() - cache = Cache(fileSystem, "/cache/".toPath(), Long.MAX_VALUE) + cache = Cache(fileSystem, cachePath, Long.MAX_VALUE) client = clientTestRule.newClientBuilder() .cache(cache) @@ -103,6 +104,7 @@ class CacheTest { @AfterEach fun tearDown() { ResponseCache.setDefault(null) + cache.close() cache.delete() } @@ -2758,7 +2760,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} writeFile(cache.directoryPath, "$urlKey.0", entryMetadata) writeFile(cache.directoryPath, "$urlKey.1", entryBody) writeFile(cache.directoryPath, "journal", journalBody) - cache = Cache(fileSystem, cache.directory.path.toPath(), Int.MAX_VALUE.toLong()) + cache = Cache(fileSystem, cachePath, Int.MAX_VALUE.toLong()) client = client.newBuilder() .cache(cache) @@ -2807,7 +2809,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} writeFile(cache.directoryPath, "$urlKey.1", entryBody) writeFile(cache.directoryPath, "journal", journalBody) cache.close() - cache = Cache(fileSystem, cache.directory.path.toPath(), Int.MAX_VALUE.toLong()) + cache = Cache(fileSystem, cachePath, Int.MAX_VALUE.toLong()) client = client.newBuilder() .cache(cache) @@ -2860,7 +2862,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} writeFile(cache.directoryPath, "$urlKey.1", entryBody) writeFile(cache.directoryPath, "journal", journalBody) cache.close() - cache = Cache(fileSystem, cache.directory.path.toPath(), Int.MAX_VALUE.toLong()) + cache = Cache(fileSystem, cachePath, Int.MAX_VALUE.toLong()) client = client.newBuilder() .cache(cache) @@ -2905,7 +2907,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} writeFile(cache.directoryPath, "$urlKey.1", entryBody) writeFile(cache.directoryPath, "journal", journalBody) cache.close() - cache = Cache(fileSystem, cache.directory.path.toPath(), Int.MAX_VALUE.toLong()) + cache = Cache(fileSystem, cachePath, Int.MAX_VALUE.toLong()) client = client.newBuilder() .cache(cache) @@ -3473,6 +3475,8 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} @Test fun testPublicPathConstructor() { + cache.close() + val events: MutableList = ArrayList() fileSystem.createDirectories(cache.directoryPath) fileSystem.createDirectories(cache.directoryPath) @@ -3495,25 +3499,24 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} return path } } - val path: Path = "/cache".toPath() - val c = Cache(loggingFileSystem, path, 100000L) - assertThat(c.directoryPath).isEqualTo(path) - c.size() + cache = Cache(loggingFileSystem, cachePath, 100000L) + assertThat(cache.directoryPath).isEqualTo(cachePath) + cache.size() assertThat(events).containsExactly( - "metadataOrNull:/cache/journal.bkp", - "metadataOrNull:/cache", - "sink:/cache/journal.bkp", - "delete:/cache/journal.bkp", - "metadataOrNull:/cache/journal", - "metadataOrNull:/cache", - "sink:/cache/journal.tmp", - "metadataOrNull:/cache/journal", - "atomicMove:/cache/journal.tmp", - "atomicMove:/cache/journal", - "appendingSink:/cache/journal", + "metadataOrNull:$cachePath/journal.bkp", + "metadataOrNull:$cachePath", + "sink:$cachePath/journal.bkp", + "delete:$cachePath/journal.bkp", + "metadataOrNull:$cachePath/journal", + "metadataOrNull:$cachePath", + "sink:$cachePath/journal.tmp", + "metadataOrNull:$cachePath/journal", + "atomicMove:$cachePath/journal.tmp", + "atomicMove:$cachePath/journal", + "appendingSink:$cachePath/journal", ) events.clear() - c.size() + cache.size() assertThat(events).isEmpty() } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/DuplexTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/DuplexTest.kt index 6f7f3384e9d0..6a63a63d1289 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/DuplexTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/DuplexTest.kt @@ -412,6 +412,9 @@ class DuplexTest { */ @Test fun duplexWithAuthChallenge() { + // TODO https://github.com/square/okhttp/issues/8342 + assumeNotWindows() + enableProtocol(Protocol.HTTP_2) val credential = basic("jesse", "secret") client = diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/LockTestProgram.java b/okhttp/src/jvmTest/kotlin/okhttp3/LockTestProgram.java new file mode 100644 index 000000000000..aa3f25a6e770 --- /dev/null +++ b/okhttp/src/jvmTest/kotlin/okhttp3/LockTestProgram.java @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2024 Block, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package okhttp3; + +import java.io.File; +import java.io.IOException; +import java.nio.channels.FileChannel; +import java.nio.file.StandardOpenOption; + +/** + * Used in CacheLockTest via Java command. + */ +public class LockTestProgram { + public static void main(String[] args) throws IOException { + File lockFile = new File(args[0]); + + System.out.println("Locking " + lockFile); + + FileChannel channel = FileChannel.open(lockFile.toPath(), StandardOpenOption.APPEND); + + channel.lock(); + + System.out.println("Locked " + lockFile); + + System.in.read(); + } +} diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/ServerTruncatesRequestTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/ServerTruncatesRequestTest.kt index 1f8a28fe67d0..199648f7e31d 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/ServerTruncatesRequestTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/ServerTruncatesRequestTest.kt @@ -26,6 +26,7 @@ import mockwebserver3.MockResponse import mockwebserver3.MockWebServer import mockwebserver3.SocketPolicy.DoNotReadRequestBody import okhttp3.Headers.Companion.headersOf +import okhttp3.TestUtil.assumeNotWindows import okhttp3.internal.duplex.AsyncRequestBody import okhttp3.internal.http2.ErrorCode import okhttp3.testing.PlatformRule @@ -69,6 +70,9 @@ class ServerTruncatesRequestTest { @Test fun serverTruncatesRequestOnLongPostHttp1() { + // SocketException: An established connection was aborted by the software in your host machine + assumeNotWindows() + serverTruncatesRequestOnLongPost(https = false) } @@ -171,6 +175,9 @@ class ServerTruncatesRequestTest { @Test fun serverTruncatesRequestButTrailersCanStillBeReadHttp1() { + // SocketException: An established connection was aborted by the software in your host machine + assumeNotWindows() + serverTruncatesRequestButTrailersCanStillBeRead(http2 = false) } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache/DiskLruCacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache/DiskLruCacheTest.kt index 284d2d9fe893..7e1f31d902ae 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache/DiskLruCacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache/DiskLruCacheTest.kt @@ -41,7 +41,9 @@ import okio.buffer import okio.fakefilesystem.FakeFileSystem import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assumptions +import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Tag +import org.junit.jupiter.api.TestInfo import org.junit.jupiter.api.Timeout import org.junit.jupiter.api.io.TempDir import org.junit.jupiter.params.ParameterizedTest @@ -59,6 +61,7 @@ class FileSystemParamProvider : SimpleProvider() { @Timeout(60) @Tag("Slow") class DiskLruCacheTest { + private lateinit var testInfo: TestInfo private lateinit var filesystem: FaultyFileSystem private var windows: Boolean = false @@ -78,7 +81,15 @@ class DiskLruCacheTest { private fun createNewCacheWithSize(maxSize: Int) { cache = - DiskLruCache(filesystem, cacheDir, appVersion, 2, maxSize.toLong(), taskRunner).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = maxSize.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } synchronized(cache) { cache.initialize() } @@ -89,7 +100,7 @@ class DiskLruCacheTest { windows: Boolean, ) { this.cacheDir = - if (baseFilesystem is FakeFileSystem) "/cache".toPath() else cacheDirFile.path.toPath() + if (baseFilesystem is FakeFileSystem) "/cache-${testInfo.testMethod.get().name}".toPath() else cacheDirFile.path.toPath() this.filesystem = FaultyFileSystem(baseFilesystem) this.windows = windows @@ -101,6 +112,11 @@ class DiskLruCacheTest { createNewCache() } + @BeforeEach + fun setup(testInfo: TestInfo) { + this.testInfo = testInfo + } + @AfterEach fun tearDown() { while (!toClose.isEmpty()) { toClose.pop().close() @@ -130,11 +146,22 @@ class DiskLruCacheTest { it.writeUtf8("Hello") } + // force close to test existing behaviour + cache.cacheLock.close() + // 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).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } assertFailsWith { @@ -249,6 +276,9 @@ class DiskLruCacheTest { creator.setString(1, "B") creator.commit() + // force close to test existing behaviour + cache.cacheLock.close() + // Simulate a dirty close of 'cache' by opening the cache directory again. createNewCache() cache["k1"]!!.use { @@ -257,6 +287,20 @@ class DiskLruCacheTest { } } + @ParameterizedTest + @ArgumentsSource(FileSystemParamProvider::class) + fun readAndWriteEntryWithoutProperCloseStoppedByLock(parameters: Pair) { + setUp(parameters.first, parameters.second) + val creator = cache.edit("k1")!! + creator.setString(0, "A") + creator.setString(1, "B") + creator.commit() + + assertFailsWith { + createNewCache() + } + } + @ParameterizedTest @ArgumentsSource(FileSystemParamProvider::class) fun journalWithEditAndPublish(parameters: Pair) { @@ -788,7 +832,15 @@ class DiskLruCacheTest { fun constructorDoesNotAllowZeroCacheSize(parameters: Pair) { setUp(parameters.first, parameters.second) assertFailsWith { - DiskLruCache(filesystem, cacheDir, appVersion, 2, 0, taskRunner) + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = 0, + taskRunner = taskRunner, + useCacheLock = true, + ) } } @@ -797,7 +849,15 @@ class DiskLruCacheTest { fun constructorDoesNotAllowZeroValuesPerEntry(parameters: Pair) { setUp(parameters.first, parameters.second) assertFailsWith { - DiskLruCache(filesystem, cacheDir, appVersion, 0, 10, taskRunner) + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 0, + maxSize = 10, + taskRunner = taskRunner, + useCacheLock = true, + ) } } @@ -1135,7 +1195,15 @@ class DiskLruCacheTest { cache.close() val dir = (cacheDir / "testOpenCreatesDirectoryIfNecessary").also { filesystem.createDirectories(it) } cache = - DiskLruCache(filesystem, dir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also { + DiskLruCache( + fileSystem = filesystem, + directory = dir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } set("a", "a", "a") @@ -1529,7 +1597,15 @@ class DiskLruCacheTest { setUp(parameters.first, parameters.second) // Create an uninitialized cache. cache = - DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } assertThat(cache.isClosed()).isFalse() @@ -1555,7 +1631,15 @@ 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).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } assertValue("a", "a", "a") @@ -1589,7 +1673,15 @@ 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).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } assertValue("a", "a", "a") @@ -1619,7 +1711,15 @@ 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).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } assertValue("a", "a", "a") @@ -1643,7 +1743,15 @@ class DiskLruCacheTest { filesystem.setFaultyWrite(journalFile, false) cache.close() cache = - DiskLruCache(filesystem, cacheDir, appVersion, 2, Int.MAX_VALUE.toLong(), taskRunner).also { + DiskLruCache( + fileSystem = filesystem, + directory = cacheDir, + appVersion = appVersion, + valueCount = 2, + maxSize = Int.MAX_VALUE.toLong(), + taskRunner = taskRunner, + useCacheLock = true, + ).also { toClose.add(it) } assertAbsent("a") diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/FileOperatorTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/FileOperatorTest.kt index ef18c3b155d4..331e64f80cef 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/FileOperatorTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/FileOperatorTest.kt @@ -27,6 +27,7 @@ import okio.ByteString.Companion.encodeUtf8 import okio.buffer import okio.sink import okio.source +import okio.use import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test @@ -34,9 +35,9 @@ import org.junit.jupiter.api.io.TempDir class FileOperatorTest { @TempDir - var tempDir: File? = null - private var file: File? = null - private var randomAccessFile: RandomAccessFile? = null + lateinit var tempDir: File + private lateinit var file: File + private lateinit var randomAccessFile: RandomAccessFile @BeforeEach fun setUp() { @@ -46,7 +47,7 @@ class FileOperatorTest { @AfterEach fun tearDown() { - randomAccessFile!!.close() + randomAccessFile.close() } @Test @@ -54,7 +55,7 @@ class FileOperatorTest { write("Hello, World".encodeUtf8()) val operator = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val buffer = Buffer() operator.read(0, buffer, 5) @@ -65,9 +66,10 @@ class FileOperatorTest { @Test fun write() { + val fileChannel = randomAccessFile.getChannel() val operator = FileOperator( - randomAccessFile!!.getChannel(), + fileChannel, ) val buffer1 = Buffer().writeUtf8("Hello, World") operator.write(0, buffer1, 5) @@ -76,13 +78,14 @@ class FileOperatorTest { operator.write(3, buffer2, 7) assertThat(buffer2.readUtf8()).isEqualTo("!") assertThat(snapshot()).isEqualTo("Helicopter".encodeUtf8()) + fileChannel.close() } @Test fun readAndWrite() { val operator = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) write("woman god creates dinosaurs destroys. ".encodeUtf8()) val buffer = Buffer() @@ -120,11 +123,11 @@ class FileOperatorTest { fun multipleOperatorsShareOneFile() { val operatorA = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val operatorB = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val bufferA = Buffer() val bufferB = Buffer() @@ -148,7 +151,7 @@ class FileOperatorTest { write(data) val operator = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val buffer = Buffer() operator.read(0, buffer, data.size.toLong()) @@ -160,7 +163,7 @@ class FileOperatorTest { val data = randomByteString(1000000) val operator = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val buffer = Buffer().write(data) operator.write(0, buffer, data.size.toLong()) @@ -171,7 +174,7 @@ class FileOperatorTest { fun readBounds() { val operator = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val buffer = Buffer() assertFailsWith { @@ -183,7 +186,7 @@ class FileOperatorTest { fun writeBounds() { val operator = FileOperator( - randomAccessFile!!.getChannel(), + randomAccessFile.getChannel(), ) val buffer = Buffer().writeUtf8("abc") assertFailsWith { @@ -201,14 +204,14 @@ class FileOperatorTest { } private fun snapshot(): ByteString { - randomAccessFile!!.getChannel().force(false) - val source = file!!.source().buffer() - return source.readByteString() + randomAccessFile.getChannel().force(false) + return file.source().buffer().use { it.readByteString() } } private fun write(data: ByteString) { - val sink = file!!.sink().buffer() - sink.write(data) - sink.close() + val sink = + file.sink().buffer().use { + it.write(data) + } } } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/RelayTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/RelayTest.kt index da28c0c117e2..d8dbb9c107dc 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/RelayTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/internal/cache2/RelayTest.kt @@ -39,11 +39,12 @@ import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.CleanupMode import org.junit.jupiter.api.io.TempDir @Tag("Slowish") class RelayTest { - @TempDir + @TempDir(cleanup = CleanupMode.ON_SUCCESS) var tempDir: File? = null private val executor = Executors.newCachedThreadPool(threadFactory("RelayTest")) private val metadata: ByteString = "great metadata!".encodeUtf8()