Lock .skiko directory using lockfile when unpacking the native binary (#1128)

In the compose hot reload project, we have seen races from multiple
tests trying to unpack skiko into the .skiko directory. Currently, the
library loading is only synchronized within the project. This MR also
adds a .lock file, which has to be locked when modifying the dataDir.

Note: This MR also changed the Library.load method from always entering
a monitor, to resolving an atomic reference, best case.

## Release Notes
### Fixes - Desktop
- Fixed a race condition that occurred when multiple processes attempted
to unpack Skiko binary files at startup

Author: sellmair

skiko/buildSrc/src/main/kotlin/tasks/configuration/JvmTasksConfiguration.kt

@@ -515,6 +515,7 @@ fun SkikoProjectContext.setupJvmTestTask(skikoAwtJarForTests: TaskProvider<Jar>,
             }
         }
 
+        classpath += files(skikoAwtRuntimeJarForTests)
         jvmArgs = listOf("--add-opens", "java.desktop/sun.font=ALL-UNNAMED")
     }
 }

skiko/src/awtTest/kotlin/org/jetbrains/skiko/LibraryLoadStressTest.kt

@@ -0,0 +1,140 @@
+@file:OptIn(ExperimentalCoroutinesApi::class)
+
+package org.jetbrains.skiko
+
+
+import kotlinx.coroutines.*
+import kotlinx.coroutines.future.asDeferred
+import kotlinx.coroutines.selects.onTimeout
+import kotlinx.coroutines.selects.select
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+import java.nio.file.Files
+import java.nio.file.Path
+import kotlin.io.path.*
+import kotlin.test.fail
+import kotlin.time.Duration.Companion.minutes
+import kotlin.time.Duration.Companion.seconds
+
+/**
+ * Special test which tests if loading the native library works 'under stress',
+ * where multiple processes try to launch at once, trying to load (and potentially unpack) skiko.
+ * The test therefore spawns multiple processes in parallel:
+ * Process A is called 'loader' which just tries to load (potentially unpack) the library
+ * Process B is called 'deleter' which deletes the .skiko folder
+ *
+ * Since multiple processes A are launched in parallel, at any given time,
+ * the 'deleter' provokes the 'loader' processes to race with each other.
+ * If not properly synchronized, those loaders will fail because of corrupted .skiko directories
+ *
+ */
+@ExperimentalPathApi
+class LibraryLoadStressTest {
+
+    /**
+     * Testing if library loading works if the 'skiko data dir' is not yet present on disk
+     */
+    @Test
+    fun `load library in empty directory`() = runTest {
+        val tempDataDir = Files.createTempDirectory("skiko-tests")
+        val nonExistingDir = tempDataDir.resolve("non-existing-dir")
+        coroutineContext.job.invokeOnCompletion { tempDataDir.deleteRecursively() }
+        launchProcess("load", nonExistingDir)
+    }
+
+    @Test
+    fun `load library - stress test`() = runTest(timeout = 10.minutes) {
+        val tempDataDir = Files.createTempDirectory("skiko-stress-test")
+        coroutineContext.job.invokeOnCompletion { tempDataDir.deleteRecursively() }
+
+        val parallelism = 4
+        val repetitions = 32
+        repeat(repetitions) { repetitionIdx ->
+            Logger.info { "running test repetition #$repetitionIdx" }
+
+            coroutineScope {
+                repeat(parallelism) { loaderIdx ->
+                    launch(Dispatchers.IO + CoroutineName("loader: $loaderIdx, repetition: $repetitionIdx")) {
+                        launchProcess("load", tempDataDir)
+                    }
+                }
+            }
+
+            withContext(Dispatchers.IO) {
+                Logger.info { "Cleanup" }
+                launchProcess("delete", tempDataDir)
+            }
+        }
+    }
+
+    suspend fun launchProcess(
+        command: String, skikoDataDir: Path
+    ) = withContext(Dispatchers.IO) {
+        val process = ProcessBuilder(
+            ProcessHandle.current().info().command().get(),
+            "-cp", System.getProperty("java.class.path"),
+            "-Dskiko.data.path=${skikoDataDir.toAbsolutePath()}",
+            "-Xmx256m", "-Xms64m",
+            StressTestMain::class.java.name, command
+        ).start()
+
+        coroutineContext.job.invokeOnCompletion {
+            process.destroyForcibly()
+        }
+
+        launch(Dispatchers.IO) {
+            process.inputStream.bufferedReader().forEachLine { line ->
+                println("$command: $line")
+            }
+        }
+
+        val errorOut = async(Dispatchers.IO) {
+            process.errorStream.bufferedReader().readText()
+        }
+
+        select {
+            process.onExit().asDeferred().onAwait { }
+            onTimeout(15.seconds) {
+                process.destroyForcibly()
+                fail("Timeout waiting on '$command' to finish")
+            }
+        }
+
+        if (process.exitValue() != 0) {
+            fail("Process ($command) exited with code ${process.exitValue()}: ${errorOut.await()}")
+        }
+    }
+
+    /**
+     * This class is the target for spawning new processes:
+     * It can act as 'loader' or 'deleter' depending on the argument ("load" or "delete")
+     */
+    object StressTestMain {
+        @JvmStatic
+        fun main(args: Array<String>) {
+            when (val operation = args.first()) {
+                "load" -> load()
+                "delete" -> delete()
+                else -> error("Operation not supported: '$operation'")
+            }
+        }
+
+        private fun load() {
+            Library.load()
+            /*
+            Check if we can access the 'currentSystemTheme' as this will perform an actual native call
+             */
+            currentSystemTheme
+        }
+
+        @OptIn(ExperimentalPathApi::class)
+        private fun delete() {
+            val dataDir = Path(SkikoProperties.dataPath)
+            if (dataDir.exists()) {
+                dataDir.listDirectoryEntries().forEach { entry ->
+                    entry.deleteRecursively()
+                }
+            }
+        }
+    }
+}

skiko/src/jvmMain/kotlin/org/jetbrains/skia/impl/Library.jvm.kt

@@ -1,18 +1,15 @@
 package org.jetbrains.skia.impl
 
 import org.jetbrains.skiko.Library
-import java.util.concurrent.atomic.AtomicBoolean
 
 actual class Library {
     actual companion object {
-        var loaded = AtomicBoolean(false)
         @JvmStatic
         actual fun staticLoad() {
-            if (loaded.compareAndSet(false, true)) {
-              Library.load()
-            }
+            Library.load()
         }
 
-        @JvmStatic external fun _nAfterLoad()
+        @JvmStatic
+        external fun _nAfterLoad()
     }
 }

skiko/src/jvmMain/kotlin/org/jetbrains/skiko/Library.kt

@@ -2,9 +2,18 @@ package org.jetbrains.skiko
 
 import org.jetbrains.skia.Bitmap
 import java.io.File
+import java.nio.channels.FileChannel
 import java.nio.file.Files
+import java.nio.file.Path
 import java.nio.file.StandardCopyOption
-import java.util.concurrent.atomic.AtomicBoolean
+import java.nio.file.StandardOpenOption.*
+import java.util.concurrent.atomic.AtomicReference
+import java.util.concurrent.locks.ReentrantLock
+import kotlin.concurrent.withLock
+import kotlin.io.bufferedReader
+import kotlin.io.path.createParentDirectories
+import kotlin.io.resolve
+import kotlin.use
 
 object Library {
     private var copyDir: File? = null
@@ -31,38 +40,66 @@ object Library {
     private fun unpackIfNeeded(dest: File, resourceName: String, deleteOnExit: Boolean): File {
         val file = File(dest, resourceName)
         if (!file.exists()) {
-            val tempFile = File.createTempFile("skiko", "", dest)
-            if (deleteOnExit)
-                file.deleteOnExit()
-            Library::class.java.getResourceAsStream("/$resourceName").use { input ->
-                Files.copy(input, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING)
+            withFileLock(dest.resolve(".lock").toPath()) {
+                if (file.exists()) return file
+                val tempFile = File.createTempFile("skiko", "", dest)
+                if (deleteOnExit)
+                    file.deleteOnExit()
+                Library::class.java.getResourceAsStream("/$resourceName").use { input ->
+                    Files.copy(input, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING)
+                }
+                Files.move(tempFile.toPath(), file.toPath(), StandardCopyOption.ATOMIC_MOVE)
             }
-            Files.move(tempFile.toPath(), file.toPath(), StandardCopyOption.ATOMIC_MOVE)
         }
         return file
     }
 
-    private var loaded = AtomicBoolean(false)
+    /**
+     * Holds a reference to the lock which has to be acquired when loading the native library,
+     * or to wait for the loading to finish.
+     * The reference will resolve to `null` if loading is done and callers do not need to wait anymore.
+     */
+    private val loadingLock = AtomicReference(ReentrantLock())
 
     // This function does the following: on request to load given resource,
     // it checks if resource with given name is found in content-derived directory
     // in Skiko's home, and if not - unpacks it. It could also load additional
     // localization resources, on platforms where it is needed.
-    @Synchronized
     fun load() {
-        if (!loaded.compareAndSet(false, true)) return
-
-        // Find/unpack a usable copy of the native library.
-        findAndLoad()
-
-        // TODO move properties to SkikoProperties
-        Setup.init()
-
-        try {
-            // Init code executed after library was loaded.
-            org.jetbrains.skia.impl.Library._nAfterLoad()
-        } catch (t: Throwable) {
-            t.printStackTrace()
+        /**
+         * If there is no more loading lock available, then the loading has finished
+         * and we can just return as normal, assuming that the library was successfully loaded.
+         */
+        val lock = loadingLock.get() ?: return
+
+        /**
+         * If the lock is held by the current thread, then this indicates a recursive call to load.
+         * Methods like `_nAfterLoad()` might trigger additional Class loading, where .clinit (static init methods)
+         * trigger further calls to Library.staticLoad() -> Library.load() while holding the current lock.
+         *
+         * It is fine, in such cases, to return eagerly and assume that the Library is successfully loaded and
+         * the recursion is a result of callbacks indicating the successful load.
+         */
+        if (lock.isHeldByCurrentThread) return
+
+        lock.withLock {
+            // We entered the critical section, but another thread might have already entered and finished
+            if (loadingLock.get() !== lock) return
+
+            // Find/unpack a usable copy of the native library.
+            findAndLoad()
+
+            // TODO move properties to SkikoProperties
+            Setup.init()
+
+            try {
+                // Init code executed after library was loaded.
+                org.jetbrains.skia.impl.Library._nAfterLoad()
+            } catch (t: Throwable) {
+                t.printStackTrace()
+            } finally {
+                loadingLock.compareAndSet(lock, null)
+            }
         }
     }
 
@@ -128,3 +165,19 @@ internal class LibraryTestImpl() {
         return bitmap._ptr
     }
 }
+
+
+/**
+ * Simple lockfile utility which ensures that the lockfile at the given [path] exists and is locked properly.
+ * Note: This method cannot be re-entered recusrively
+ * Note: The same process can only take a given lock once
+ */
+internal inline fun <T> withFileLock(path: Path, action: () -> T): T {
+    path.createParentDirectories()
+    return FileChannel.open(path, READ, WRITE, CREATE).use { channel ->
+        val lock = channel.lock()
+        lock.use {
+            action()
+        }
+    }
+}