From f65f8d25f65b37d70f4260e3d91ba0879fd3cfae Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 17:19:14 -0700 Subject: [PATCH 01/28] Update the minimum yml changes to make ci working --- .github/workflows/main.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index df0a981..26db3e7 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -45,15 +45,15 @@ jobs: - name: Run tests run: ./gradlew test --stacktrace - name: Run instrumentation tests - uses: malinskiy/action-android/emulator-run-cmd@release/0.1.0 + uses: malinskiy/action-android/emulator-run-cmd@release/0.1.7 with: cmd: ./gradlew connectedCheck --stacktrace - api: 21 + api: 30 tag: default abi: x86 - name: Full check run: ./gradlew check --stacktrace - - uses: actions/upload-artifact@v2 + - uses: actions/upload-artifact@v4 with: name: Sample App path: sample/build/outputs From 27ed6e75805173c034d67e2d6f22f97cfeeb5f68 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 17:34:46 -0700 Subject: [PATCH 02/28] Update the minimum gradle changes to support android api 30 Robolectric test --- gradle/dependencies.gradle | 2 +- simplestore/build.gradle | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 83fa51e..fe580c4 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -56,7 +56,7 @@ def test = [ junitX : 'androidx.test.ext:junit:1.0.0', truth : 'com.google.truth:truth:1.1', truthX : 'androidx.test.ext:truth:1.5.0', - robolectric: 'org.robolectric:robolectric:4.4', + robolectric: 'org.robolectric:robolectric:4.5.1', espresso : 'androidx.test.espresso:espresso-core:3.5.1', runner : 'androidx.test:runner:1.5.2', rules : 'androidx.test:rules:1.5.0', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index bdef651..d560b7b 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -23,6 +23,9 @@ android { testOptions { execution 'ANDROIDX_TEST_ORCHESTRATOR' + unitTests { + includeAndroidResources = true + } } defaultConfig { From e643582cf4837618e3e72b3b5c03f0d2dc0dfbde Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 17:36:01 -0700 Subject: [PATCH 03/28] Add rxjava2 as test dependencies --- gradle/dependencies.gradle | 1 + simplestore/build.gradle | 1 + 2 files changed, 2 insertions(+) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index fe580c4..3f1854e 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -57,6 +57,7 @@ def test = [ truth : 'com.google.truth:truth:1.1', truthX : 'androidx.test.ext:truth:1.5.0', robolectric: 'org.robolectric:robolectric:4.5.1', + rxjava2: 'io.reactivex.rxjava2:rxjava:2.2.21', espresso : 'androidx.test.espresso:espresso-core:3.5.1', runner : 'androidx.test:runner:1.5.2', rules : 'androidx.test:rules:1.5.0', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index d560b7b..f218aef 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -49,6 +49,7 @@ dependencies { testImplementation deps.test.truth testImplementation deps.test.truthX testImplementation deps.test.robolectric + testImplementation deps.test.rxjava2 androidTestImplementation deps.test.runner androidTestImplementation deps.test.rules From 16bb64b566d5c9b3a32076ca4067d8f74b648c19 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 17:38:20 -0700 Subject: [PATCH 04/28] Add more SimpleStore and AtomicFile integration test --- .../impl/AtomicFileIntegrationTest.kt | 65 +++++++++++++++ .../impl/PrimitiveSimpleStoreRepo.kt | 67 +++++++++++++++ .../impl/PrimitiveSimpleStoreTestUtil.kt | 32 ++++++++ .../impl/SimpleStoreIntegrationTest.kt | 82 +++++++++++++++++++ 4 files changed, 246 insertions(+) create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt new file mode 100644 index 0000000..19ee662 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File +import java.io.FileNotFoundException +import java.io.FileOutputStream +import java.nio.charset.StandardCharsets + +@RunWith(RobolectricTestRunner::class) +class AtomicFileIntegrationTest { + + @Test + fun `case 0 when value written will be persisted as an independent file`() { + val file = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + val fileOutputStream = fileOutputStream(file) + fileOutputStream.write("123".toByteArray(StandardCharsets.UTF_8)) + fileOutputStream.close() + val atomicFile = AtomicFile(file) + val readFully = atomicFile.readFully() + Truth.assertThat(readFully).isEqualTo("123".toByteArray(StandardCharsets.UTF_8)) + } + + private fun fileOutputStream(file: File): FileOutputStream { + return try { + file.outputStream() + } catch (e: FileNotFoundException) { + val parentFile = file.parentFile ?: throw e + val failed = checkWithLock(parentFile) + if (failed) { + throw e + } else { + file.outputStream() + } + } + } + + @Synchronized + private fun checkWithLock(parentFile: File): Boolean { + return !parentFile.exists() && !parentFile.mkdirs() + } + + private fun app(): Application { + return ApplicationProvider.getApplicationContext() + } +} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt b/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt new file mode 100644 index 0000000..995d10f --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import android.app.Application +import com.google.common.util.concurrent.ListenableFuture +import com.uber.simplestore.NamespaceConfig +import com.uber.simplestore.primitive.PrimitiveSimpleStore +import com.uber.simplestore.primitive.PrimitiveSimpleStoreFactory +import io.reactivex.Single + +internal class PrimitiveSimpleStoreRepo(app: Application) { + + private val dataStore = dataStore(app) + + fun writeSingle(counter: Long, key: String): Single { + return Single.fromFuture(listenableFuture(key, counter)) + } + + + private fun listenableFuture(key: String, counter: Long): ListenableFuture { + return dataStore.put(key, counter) + } + + fun writeString(value: String, key: String): Single { + return Single.fromFuture(dataStore.putString(key, value)) + } + + fun readSingle(key: String): Single { + return Single.fromFuture(dataStore.getLong(key)) + } + + /** No more read fallback as it is new API added. */ + fun readString(key: String): Single { + return Single.fromFuture(dataStore.getString(key)) + } + + + fun close() { + dataStore.close() + } + + companion object { + + private fun dataStore(app: Application): PrimitiveSimpleStore { + return PrimitiveSimpleStoreFactory.create( + AndroidDirectoryProvider(app), + "657b3cd7-f689-451b-aca0-628de60aa234", + NamespaceConfig.CRITICAL + ) + } + + } +} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt new file mode 100644 index 0000000..796c592 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt @@ -0,0 +1,32 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + + +object PrimitiveSimpleStoreTestUtil { + + fun transformToLong(b: ByteArray): Long { + return b[0].toLong() and 0xFFL shl 56 or + (b[1].toLong() and 0xFFL shl 48) or + (b[2].toLong() and 0xFFL shl 40) or + (b[3].toLong() and 0xFFL shl 32) or + (b[4].toLong() and 0xFFL shl 24) or + (b[5].toLong() and 0xFFL shl 16) or + (b[6].toLong() and 0xFFL shl 8) or + (b[7].toLong() and 0xFFL) + } + +} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt new file mode 100644 index 0000000..7806f33 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt @@ -0,0 +1,82 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File +import java.nio.charset.StandardCharsets + +@RunWith(RobolectricTestRunner::class) +class SimpleStoreIntegrationTest { + + @Test + fun `case 0 when value written will be persisted as an independent file`() { + generateFile() + assertFile1() + assertFile2() + } + + private fun generateFile() { + val repo = repo() + val test1 = repo.writeSingle(123, "file1").test() + val test2 = repo.writeString("456", "file2").test() + test1.assertValue(123) + test2.assertValue("456") + repo.close() + } + + private fun assertFile1() { + File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/file1").apply { + assertThat(exists()).isTrue() + val atomicFile = AtomicFile(this) + val readFully = atomicFile.readFully() + val bytes = readFully + val value = PrimitiveSimpleStoreTestUtil.transformToLong(bytes) + assertThat(value).isNotEqualTo("123") + assertThat(value).isEqualTo(123) + val repo = repo() + repo.readSingle("file1").test().assertValue(123) + repo.close() + } + } + + private fun assertFile2() { + File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/file2").apply { + assertThat(exists()).isTrue() + val atomicFile = AtomicFile(this) + val readFully = atomicFile.readFully() + val bytes = readFully + val value =String(bytes, StandardCharsets.UTF_16BE) + assertThat(value).isEqualTo("456") + val repo = repo() + repo.readString("file2").test().assertValue("456") + repo.close() + } + } + + private fun repo(): PrimitiveSimpleStoreRepo { + return PrimitiveSimpleStoreRepo(app()) + } + + private fun app(): Application { + return ApplicationProvider.getApplicationContext() + } +} From dbad1e53c6ced55afa226ee7d2a2d83e23200655 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 17:42:42 -0700 Subject: [PATCH 05/28] Fix the race condition issue on creating the parent folder. --- .../com/uber/simplestore/impl/AtomicFile.java | 54 ++++++++++++++++--- 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index 2273d7a..9d6f704 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -16,6 +16,8 @@ package com.uber.simplestore.impl; import android.util.Log; +import com.google.common.annotations.VisibleForTesting; + import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -45,6 +47,8 @@ final class AtomicFile { private static final String LOG_TAG = "AtomicFile"; + private static final Object lock = new Object(); + private final File mBaseName; private final File mNewName; private final File mLegacyBackupName; @@ -89,22 +93,56 @@ public FileOutputStream startWrite() throws IOException { if (mLegacyBackupName.exists()) { rename(mLegacyBackupName, mBaseName); } + return createFileOutputStreamForWrite(true); + } + private FileOutputStream createParentLegacy(FileNotFoundException e) throws IOException { + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw new IOException("Failed to create directory for " + mNewName); + } + try { + return new FileOutputStream(mNewName); + } catch (FileNotFoundException e2) { + throw new IOException("Failed to create new file " + mNewName, e2); + } + } + + @VisibleForTesting + FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationException) throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw new IOException("Failed to create directory for " + mNewName); - } - try { - return new FileOutputStream(mNewName); - } catch (FileNotFoundException e2) { - throw new IOException("Failed to create new file " + mNewName, e2); + return createParent(e, fixOnFileNotFoundationException); + } + } + + private FileOutputStream createParent(FileNotFoundException rawError, boolean fixOnFileNotFoundationException) throws IOException { + if (fixOnFileNotFoundationException) { + return createParentWithFix(rawError); + } else { + return createParentLegacy(rawError); + } + } + + private FileOutputStream createParentWithFix(FileNotFoundException rawError) throws IOException { + File parent = mNewName.getParentFile(); + if (parent == null) { + throw rawError; + } + synchronized (lock) { + if (!parent.exists() && !parent.mkdirs()) { + throw rawError; } } + try { + return new FileOutputStream(mNewName); + } catch (FileNotFoundException e2) { + throw new IOException("Failed to create new file " + mNewName, e2); + } } + /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From 3997ec002bb479cabb86cbc72651671980b4d13d Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 17:42:42 -0700 Subject: [PATCH 06/28] Add AtomicFileConcurrentTest.kt --- .../impl/AtomicFileConcurrentTest.kt | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt new file mode 100644 index 0000000..75b60a4 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File +import java.io.FileNotFoundException + +@RunWith(RobolectricTestRunner::class) +class AtomicFileConcurrentTest { + + @Test(expected = FileNotFoundException::class) + fun `case 0 when parent folder is absent will trigger FileNotFoundException`() { + //arrange + val targetFile = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + //act + val target = targetFile.outputStream() + //assert + assertThat(target).isNotNull() + } + + @Test + fun `case 1 when parent folder is present will not trigger FileNotFoundException`() { + //arrange + makeParentFolder() + val targetFile = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + //act + val target = targetFile.outputStream() + //assert + assertThat(target).isNotNull() + } + + private fun makeParentFolder() { + val parent = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234") + parent.mkdirs() + } + + private fun app(): Application { + return ApplicationProvider.getApplicationContext() + } +} From 5cc34adf5417c487d901dee3ab9d331b74224968 Mon Sep 17 00:00:00 2001 From: TonyTang Date: Sat, 14 Sep 2024 20:54:23 -0700 Subject: [PATCH 07/28] Add AtomicFileConcurrentTest.kt that reproducing the failed exception --- gradle/dependencies.gradle | 1 + simplestore/build.gradle | 1 + .../impl/AtomicFileConcurrentTest.kt | 26 +++++++- .../simplestore/impl/ConcurrentTestUtil.kt | 60 +++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 3f1854e..1160c98 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -52,6 +52,7 @@ def androidx = [ ] def test = [ + concurrentunit : 'net.jodah:concurrentunit:0.4.6', junit : 'junit:junit:4.13.1', junitX : 'androidx.test.ext:junit:1.0.0', truth : 'com.google.truth:truth:1.1', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index f218aef..bb7e956 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -48,6 +48,7 @@ dependencies { testImplementation deps.test.junitX testImplementation deps.test.truth testImplementation deps.test.truthX + testImplementation deps.test.concurrentunit testImplementation deps.test.robolectric testImplementation deps.test.rxjava2 diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt index 75b60a4..8cfe1ba 100644 --- a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt @@ -18,19 +18,22 @@ package com.uber.simplestore.impl import android.app.Application import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat +import com.uber.simplestore.impl.ConcurrentTestUtil.executeConcurrent import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner import java.io.File import java.io.FileNotFoundException + @RunWith(RobolectricTestRunner::class) class AtomicFileConcurrentTest { @Test(expected = FileNotFoundException::class) fun `case 0 when parent folder is absent will trigger FileNotFoundException`() { //arrange - val targetFile = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + //makeParentFolder() + val targetFile = createTargetFile() //act val target = targetFile.outputStream() //assert @@ -41,13 +44,32 @@ class AtomicFileConcurrentTest { fun `case 1 when parent folder is present will not trigger FileNotFoundException`() { //arrange makeParentFolder() - val targetFile = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + val targetFile = createTargetFile() //act val target = targetFile.outputStream() //assert assertThat(target).isNotNull() } + @Test(expected = AssertionError::class) + fun `case 2 when accessed concurrently without fix it will trigger error`() { + val baseFile = createTargetFile() + val atomicFile = AtomicFile(baseFile) + executeConcurrent { atomicFile.createFileOutputStreamForWrite(false) } + } + + @Test + fun `case 3 when accessed concurrently with fix it will not trigger error`() { + val baseFile = createTargetFile() + val atomicFile = AtomicFile(baseFile) + executeConcurrent { atomicFile.createFileOutputStreamForWrite(true) } + } + + + private fun createTargetFile(): File { + return File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + } + private fun makeParentFolder() { val parent = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234") parent.mkdirs() diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt new file mode 100644 index 0000000..a38dfc4 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import net.jodah.concurrentunit.Waiter + + +/** + * A utility class to execute a given action concurrently. + * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. + * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. + */ +object ConcurrentTestUtil { + /** + * The maximum number of threads to be executed concurrently. + */ + private const val MAX_THREAD = 5 + + + /** + * Executes the given action concurrently. + * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. + * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. + */ + fun executeConcurrent(action: () -> Unit) { + val waiter = Waiter() + val threadCount = MAX_THREAD + for (i in 0 until threadCount) { + Thread { + executeConcurrentInternally(action, waiter) + }.start() + } + waiter.await(1000L * threadCount) + } + + private fun executeConcurrentInternally(action: () -> Unit, waiter: Waiter) { + try { + run(action) + } catch (e: Exception) { + waiter.fail(e) + } finally { + waiter.resume() + } + } + + +} From 868555796b7634280fc3031717f27587164ef982 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:01:38 -0700 Subject: [PATCH 08/28] Bump up the ci --- .github/workflows/main.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 26db3e7..ef9ea6d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -20,21 +20,21 @@ jobs: java_version: [11] steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4.1.7 - name: Install JDK ${{ matrix.java_version }} - uses: actions/setup-java@v1 + uses: actions/setup-java@v4.3.0 with: java-version: ${{ matrix.java_version }} - name: Gradle Wrapper Validation - uses: gradle/wrapper-validation-action@v1 + uses: gradle/wrapper-validation-action@v3.5.0 - name: Cache gradle packages - uses: actions/cache@v2 + uses: actions/cache@v4.0.2 with: path: ~/.gradle/caches key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} restore-keys: ${{ runner.os }}-gradle - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.0 + uses: malinskiy/action-android/install-sdk@release/0.1.7 - name: Configure Gradle run: ./gradlew help - name: Check formatting From 8c1bc769f46a33ad436a50a97e39e68d4237010c Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:04:45 -0700 Subject: [PATCH 09/28] Use `./gradlew spotlessApply` to fix the file format. --- .../main/java/com/uber/simplestore/impl/AtomicFile.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index 9d6f704..c596eeb 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -17,7 +17,6 @@ import android.util.Log; import com.google.common.annotations.VisibleForTesting; - import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -109,7 +108,8 @@ private FileOutputStream createParentLegacy(FileNotFoundException e) throws IOEx } @VisibleForTesting - FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationException) throws IOException { + FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationException) + throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { @@ -117,7 +117,8 @@ FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationEx } } - private FileOutputStream createParent(FileNotFoundException rawError, boolean fixOnFileNotFoundationException) throws IOException { + private FileOutputStream createParent( + FileNotFoundException rawError, boolean fixOnFileNotFoundationException) throws IOException { if (fixOnFileNotFoundationException) { return createParentWithFix(rawError); } else { @@ -142,7 +143,6 @@ private FileOutputStream createParentWithFix(FileNotFoundException rawError) thr } } - /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From 24f5f0f80f511d31a6618d3e9f99abe22151fbcd Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:09:33 -0700 Subject: [PATCH 10/28] Fix the release ci --- .github/workflows/main.yml | 1 + .github/workflows/release.yml | 13 +++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ef9ea6d..0275d39 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -24,6 +24,7 @@ jobs: - name: Install JDK ${{ matrix.java_version }} uses: actions/setup-java@v4.3.0 with: + distribution: 'temurin' java-version: ${{ matrix.java_version }} - name: Gradle Wrapper Validation uses: gradle/wrapper-validation-action@v3.5.0 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 25c891a..4f8b857 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,24 +10,25 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4.1.7 - name: Install JDK 11 - uses: actions/setup-java@v1 + uses: actions/setup-java@v4.3.0 with: + distribution: 'temurin' java-version: 11 - name: Gradle Wrapper Validation - uses: gradle/wrapper-validation-action@v1 + uses: gradle/wrapper-validation-action@v3.5.0 - name: Cache gradle packages - uses: actions/cache@v2 + uses: actions/cache@v4.0.2 with: path: ~/.gradle/caches key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} restore-keys: ${{ runner.os }}-gradle - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.0 + uses: malinskiy/action-android/install-sdk@release/0.1.7 - name: Configure Gradle run: ./gradlew help - - uses: actions/setup-python@v1 + - uses: actions/setup-python@v5.2.0 - uses: BSFishy/pip-action@v1 with: packages: | From cfe78e01d23fe9ded4619946e25998364984a5e0 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:20:15 -0700 Subject: [PATCH 11/28] Fix the main ci --- .github/workflows/main.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0275d39..7fd7eb8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,10 +1,10 @@ name: CI on: - # Only run push on master + # Only run push on main push: branches: - - master + - main paths-ignore: - 'docs/**' - '*.md' @@ -14,7 +14,7 @@ on: jobs: build: name: JDK ${{ matrix.java_version }} - runs-on: macOS-latest + runs-on: macos-12 strategy: matrix: java_version: [11] From 09c9fb8d9b35efe0c5214aed42d01ad49e1216bf Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:22:43 -0700 Subject: [PATCH 12/28] Fix the main ci --- .github/workflows/main.yml | 4 ++-- .github/workflows/release.yml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7fd7eb8..7f7562e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -14,10 +14,10 @@ on: jobs: build: name: JDK ${{ matrix.java_version }} - runs-on: macos-12 + runs-on: macos-13 strategy: matrix: - java_version: [11] + java_version: [17] steps: - name: Checkout uses: actions/checkout@v4.1.7 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4f8b857..95f8da4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -11,11 +11,11 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4.1.7 - - name: Install JDK 11 + - name: Install JDK 17 uses: actions/setup-java@v4.3.0 with: distribution: 'temurin' - java-version: 11 + java-version: 17 - name: Gradle Wrapper Validation uses: gradle/wrapper-validation-action@v3.5.0 - name: Cache gradle packages From 2ef87200836e9f2b546ad4b7fd9c2807229e245e Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:28:09 -0700 Subject: [PATCH 13/28] Fix the main ci by bumping down malinskiy/action-android --- .github/workflows/main.yml | 10 +++++----- .github/workflows/release.yml | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7f7562e..3d876c8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -14,10 +14,10 @@ on: jobs: build: name: JDK ${{ matrix.java_version }} - runs-on: macos-13 + runs-on: macos-12 strategy: matrix: - java_version: [17] + java_version: [11] steps: - name: Checkout uses: actions/checkout@v4.1.7 @@ -35,7 +35,7 @@ jobs: key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} restore-keys: ${{ runner.os }}-gradle - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.7 + uses: malinskiy/action-android/install-sdk@release/0.1.3 - name: Configure Gradle run: ./gradlew help - name: Check formatting @@ -46,12 +46,12 @@ jobs: - name: Run tests run: ./gradlew test --stacktrace - name: Run instrumentation tests - uses: malinskiy/action-android/emulator-run-cmd@release/0.1.7 + uses: malinskiy/action-android/emulator-run-cmd@release/0.1.3 with: cmd: ./gradlew connectedCheck --stacktrace api: 30 tag: default - abi: x86 + abi: x86_64 - name: Full check run: ./gradlew check --stacktrace - uses: actions/upload-artifact@v4 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 95f8da4..4e09572 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -11,11 +11,11 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4.1.7 - - name: Install JDK 17 + - name: Install JDK 11 uses: actions/setup-java@v4.3.0 with: distribution: 'temurin' - java-version: 17 + java-version: 11 - name: Gradle Wrapper Validation uses: gradle/wrapper-validation-action@v3.5.0 - name: Cache gradle packages @@ -25,7 +25,7 @@ jobs: key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} restore-keys: ${{ runner.os }}-gradle - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.7 + uses: malinskiy/action-android/install-sdk@release/0.1.3 - name: Configure Gradle run: ./gradlew help - uses: actions/setup-python@v5.2.0 From 115d2efe8f9152f60d66bca1242e9e69ee04fa9b Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:53:51 -0700 Subject: [PATCH 14/28] Fix the main ci by using RIBs --- .github/workflows/main.yml | 69 ++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 3d876c8..a3a1753 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,60 +1,49 @@ name: CI -on: - # Only run push on main - push: - branches: - - main - paths-ignore: - - 'docs/**' - - '*.md' - # Always run on PRs - pull_request: +on: [push, pull_request] jobs: build: name: JDK ${{ matrix.java_version }} - runs-on: macos-12 + runs-on: ubuntu-latest + defaults: + run: + working-directory: ./android strategy: matrix: + # TODO Add 9, 10, 11, 12, and 13 after Kotlin 1.3.60 java_version: [11] steps: - name: Checkout - uses: actions/checkout@v4.1.7 - - name: Install JDK ${{ matrix.java_version }} - uses: actions/setup-java@v4.3.0 + uses: actions/checkout@v2 + - name: Install JDK + uses: actions/setup-java@v2 with: - distribution: 'temurin' + distribution: 'zulu' java-version: ${{ matrix.java_version }} - - name: Gradle Wrapper Validation - uses: gradle/wrapper-validation-action@v3.5.0 - - name: Cache gradle packages - uses: actions/cache@v4.0.2 - with: - path: ~/.gradle/caches - key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} - restore-keys: ${{ runner.os }}-gradle - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.3 + uses: malinskiy/action-android/install-sdk@release/0.1.1 + - name: Gradle Wrapper Validation + uses: gradle/wrapper-validation-action@v1 - name: Configure Gradle + # Gradle configuration install deps run: ./gradlew help - - name: Check formatting + - name: Spot check run: ./gradlew spotlessCheck --stacktrace - - name: Build project + - name: Build Project run: ./gradlew assemble --stacktrace - # TODO split test and instrumentation into parallel builds - - name: Run tests + - name : Testing run: ./gradlew test --stacktrace - - name: Run instrumentation tests - uses: malinskiy/action-android/emulator-run-cmd@release/0.1.3 - with: - cmd: ./gradlew connectedCheck --stacktrace - api: 30 - tag: default - abi: x86_64 - - name: Full check - run: ./gradlew check --stacktrace - - uses: actions/upload-artifact@v4 + - name: Enable KVM + run: | + echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules + sudo udevadm control --reload-rules + sudo udevadm trigger --name-match=kvm + - name: run tests + uses: reactivecircus/android-emulator-runner@v2 with: - name: Sample App - path: sample/build/outputs + api-level: 29 + script: + ./gradlew connectedCheck + - name: Final Checks + run: ./gradlew check --stacktrace \ No newline at end of file From 1733e143e96246efd7929f3b5b7f0b30ea2ff128 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:54:48 -0700 Subject: [PATCH 15/28] Fix the main ci by using pure RIBs script --- .github/workflows/main.yml | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a3a1753..a849acd 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -34,16 +34,5 @@ jobs: run: ./gradlew assemble --stacktrace - name : Testing run: ./gradlew test --stacktrace - - name: Enable KVM - run: | - echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules - sudo udevadm control --reload-rules - sudo udevadm trigger --name-match=kvm - - name: run tests - uses: reactivecircus/android-emulator-runner@v2 - with: - api-level: 29 - script: - ./gradlew connectedCheck - name: Final Checks run: ./gradlew check --stacktrace \ No newline at end of file From fd3fa2fe58510c48d0cd6f8f4733c37e3eb12bc1 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:56:17 -0700 Subject: [PATCH 16/28] Only apply to pull request --- .github/workflows/main.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a849acd..aa46d2a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,14 +1,11 @@ name: CI -on: [push, pull_request] +on: [pull_request] jobs: build: name: JDK ${{ matrix.java_version }} runs-on: ubuntu-latest - defaults: - run: - working-directory: ./android strategy: matrix: # TODO Add 9, 10, 11, 12, and 13 after Kotlin 1.3.60 From 1419aa732484ab41d575f184c9cba739c9e9280c Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 21:56:17 -0700 Subject: [PATCH 17/28] Make the ConcurrentTestUtil more intensive --- .../uber/simplestore/impl/ConcurrentTestUtil.kt | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt index a38dfc4..cbdbdc4 100644 --- a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt +++ b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt @@ -16,6 +16,7 @@ package com.uber.simplestore.impl import net.jodah.concurrentunit.Waiter +import java.util.concurrent.Executors /** @@ -37,13 +38,15 @@ object ConcurrentTestUtil { */ fun executeConcurrent(action: () -> Unit) { val waiter = Waiter() - val threadCount = MAX_THREAD - for (i in 0 until threadCount) { - Thread { - executeConcurrentInternally(action, waiter) - }.start() + for (i in 0 until 1000) { + val service = Executors.newFixedThreadPool(MAX_THREAD) + for (j in 0 until 10) { + service.submit { + executeConcurrentInternally(action, waiter) + } + } } - waiter.await(1000L * threadCount) + waiter.await(1000L * MAX_THREAD) } private fun executeConcurrentInternally(action: () -> Unit, waiter: Waiter) { From a677929b9f32db52ae42574ee9ee50e930600578 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 22:16:32 -0700 Subject: [PATCH 18/28] Ignore the flaky test --- .../java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt index 8cfe1ba..2afa0ee 100644 --- a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt @@ -19,6 +19,7 @@ import android.app.Application import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat import com.uber.simplestore.impl.ConcurrentTestUtil.executeConcurrent +import org.junit.Ignore import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -51,6 +52,7 @@ class AtomicFileConcurrentTest { assertThat(target).isNotNull() } + @Ignore("failing_on_ci") @Test(expected = AssertionError::class) fun `case 2 when accessed concurrently without fix it will trigger error`() { val baseFile = createTargetFile() From 0438626e1296e10494f94d732657572b74691d3f Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 22:22:31 -0700 Subject: [PATCH 19/28] Refine the test comment --- .../java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt index 2afa0ee..0558951 100644 --- a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt @@ -52,7 +52,7 @@ class AtomicFileConcurrentTest { assertThat(target).isNotNull() } - @Ignore("failing_on_ci") + @Ignore("This is failing due to CI flakiness. One could enable it locally to reproduce the issue.") @Test(expected = AssertionError::class) fun `case 2 when accessed concurrently without fix it will trigger error`() { val baseFile = createTargetFile() From e68a3a9d233fabea71135addf31a2b92048d2123 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 22:26:55 -0700 Subject: [PATCH 20/28] Keep original changes. --- .github/workflows/main.yml | 52 ++++++++---- .github/workflows/release.yml | 13 ++- gradle/dependencies.gradle | 3 +- simplestore/build.gradle | 4 - .../com/uber/simplestore/impl/AtomicFile.java | 54 ++---------- .../impl/AtomicFileConcurrentTest.kt | 83 ------------------- .../impl/AtomicFileIntegrationTest.kt | 65 --------------- .../simplestore/impl/ConcurrentTestUtil.kt | 63 -------------- .../impl/PrimitiveSimpleStoreRepo.kt | 67 --------------- .../impl/PrimitiveSimpleStoreTestUtil.kt | 32 ------- .../impl/SimpleStoreIntegrationTest.kt | 82 ------------------ 11 files changed, 53 insertions(+), 465 deletions(-) delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index aa46d2a..df0a981 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,35 +1,59 @@ name: CI -on: [pull_request] +on: + # Only run push on master + push: + branches: + - master + paths-ignore: + - 'docs/**' + - '*.md' + # Always run on PRs + pull_request: jobs: build: name: JDK ${{ matrix.java_version }} - runs-on: ubuntu-latest + runs-on: macOS-latest strategy: matrix: - # TODO Add 9, 10, 11, 12, and 13 after Kotlin 1.3.60 java_version: [11] steps: - name: Checkout uses: actions/checkout@v2 - - name: Install JDK - uses: actions/setup-java@v2 + - name: Install JDK ${{ matrix.java_version }} + uses: actions/setup-java@v1 with: - distribution: 'zulu' java-version: ${{ matrix.java_version }} - - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.1 - name: Gradle Wrapper Validation uses: gradle/wrapper-validation-action@v1 + - name: Cache gradle packages + uses: actions/cache@v2 + with: + path: ~/.gradle/caches + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Install Android SDK + uses: malinskiy/action-android/install-sdk@release/0.1.0 - name: Configure Gradle - # Gradle configuration install deps run: ./gradlew help - - name: Spot check + - name: Check formatting run: ./gradlew spotlessCheck --stacktrace - - name: Build Project + - name: Build project run: ./gradlew assemble --stacktrace - - name : Testing + # TODO split test and instrumentation into parallel builds + - name: Run tests run: ./gradlew test --stacktrace - - name: Final Checks - run: ./gradlew check --stacktrace \ No newline at end of file + - name: Run instrumentation tests + uses: malinskiy/action-android/emulator-run-cmd@release/0.1.0 + with: + cmd: ./gradlew connectedCheck --stacktrace + api: 21 + tag: default + abi: x86 + - name: Full check + run: ./gradlew check --stacktrace + - uses: actions/upload-artifact@v2 + with: + name: Sample App + path: sample/build/outputs diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4e09572..25c891a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,25 +10,24 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v4.1.7 + uses: actions/checkout@v2 - name: Install JDK 11 - uses: actions/setup-java@v4.3.0 + uses: actions/setup-java@v1 with: - distribution: 'temurin' java-version: 11 - name: Gradle Wrapper Validation - uses: gradle/wrapper-validation-action@v3.5.0 + uses: gradle/wrapper-validation-action@v1 - name: Cache gradle packages - uses: actions/cache@v4.0.2 + uses: actions/cache@v2 with: path: ~/.gradle/caches key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} restore-keys: ${{ runner.os }}-gradle - name: Install Android SDK - uses: malinskiy/action-android/install-sdk@release/0.1.3 + uses: malinskiy/action-android/install-sdk@release/0.1.0 - name: Configure Gradle run: ./gradlew help - - uses: actions/setup-python@v5.2.0 + - uses: actions/setup-python@v1 - uses: BSFishy/pip-action@v1 with: packages: | diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 1160c98..9a219c5 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -57,8 +57,7 @@ def test = [ junitX : 'androidx.test.ext:junit:1.0.0', truth : 'com.google.truth:truth:1.1', truthX : 'androidx.test.ext:truth:1.5.0', - robolectric: 'org.robolectric:robolectric:4.5.1', - rxjava2: 'io.reactivex.rxjava2:rxjava:2.2.21', + robolectric: 'org.robolectric:robolectric:4.4', espresso : 'androidx.test.espresso:espresso-core:3.5.1', runner : 'androidx.test:runner:1.5.2', rules : 'androidx.test:rules:1.5.0', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index bb7e956..c81eda7 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -23,9 +23,6 @@ android { testOptions { execution 'ANDROIDX_TEST_ORCHESTRATOR' - unitTests { - includeAndroidResources = true - } } defaultConfig { @@ -50,7 +47,6 @@ dependencies { testImplementation deps.test.truthX testImplementation deps.test.concurrentunit testImplementation deps.test.robolectric - testImplementation deps.test.rxjava2 androidTestImplementation deps.test.runner androidTestImplementation deps.test.rules diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index c596eeb..2273d7a 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -16,7 +16,6 @@ package com.uber.simplestore.impl; import android.util.Log; -import com.google.common.annotations.VisibleForTesting; import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -46,8 +45,6 @@ final class AtomicFile { private static final String LOG_TAG = "AtomicFile"; - private static final Object lock = new Object(); - private final File mBaseName; private final File mNewName; private final File mLegacyBackupName; @@ -92,54 +89,19 @@ public FileOutputStream startWrite() throws IOException { if (mLegacyBackupName.exists()) { rename(mLegacyBackupName, mBaseName); } - return createFileOutputStreamForWrite(true); - } - - private FileOutputStream createParentLegacy(FileNotFoundException e) throws IOException { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw new IOException("Failed to create directory for " + mNewName); - } - try { - return new FileOutputStream(mNewName); - } catch (FileNotFoundException e2) { - throw new IOException("Failed to create new file " + mNewName, e2); - } - } - @VisibleForTesting - FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationException) - throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - return createParent(e, fixOnFileNotFoundationException); - } - } - - private FileOutputStream createParent( - FileNotFoundException rawError, boolean fixOnFileNotFoundationException) throws IOException { - if (fixOnFileNotFoundationException) { - return createParentWithFix(rawError); - } else { - return createParentLegacy(rawError); - } - } - - private FileOutputStream createParentWithFix(FileNotFoundException rawError) throws IOException { - File parent = mNewName.getParentFile(); - if (parent == null) { - throw rawError; - } - synchronized (lock) { - if (!parent.exists() && !parent.mkdirs()) { - throw rawError; + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw new IOException("Failed to create directory for " + mNewName); + } + try { + return new FileOutputStream(mNewName); + } catch (FileNotFoundException e2) { + throw new IOException("Failed to create new file " + mNewName, e2); } - } - try { - return new FileOutputStream(mNewName); - } catch (FileNotFoundException e2) { - throw new IOException("Failed to create new file " + mNewName, e2); } } diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt deleted file mode 100644 index 0558951..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt +++ /dev/null @@ -1,83 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import android.app.Application -import androidx.test.core.app.ApplicationProvider -import com.google.common.truth.Truth.assertThat -import com.uber.simplestore.impl.ConcurrentTestUtil.executeConcurrent -import org.junit.Ignore -import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner -import java.io.File -import java.io.FileNotFoundException - - -@RunWith(RobolectricTestRunner::class) -class AtomicFileConcurrentTest { - - @Test(expected = FileNotFoundException::class) - fun `case 0 when parent folder is absent will trigger FileNotFoundException`() { - //arrange - //makeParentFolder() - val targetFile = createTargetFile() - //act - val target = targetFile.outputStream() - //assert - assertThat(target).isNotNull() - } - - @Test - fun `case 1 when parent folder is present will not trigger FileNotFoundException`() { - //arrange - makeParentFolder() - val targetFile = createTargetFile() - //act - val target = targetFile.outputStream() - //assert - assertThat(target).isNotNull() - } - - @Ignore("This is failing due to CI flakiness. One could enable it locally to reproduce the issue.") - @Test(expected = AssertionError::class) - fun `case 2 when accessed concurrently without fix it will trigger error`() { - val baseFile = createTargetFile() - val atomicFile = AtomicFile(baseFile) - executeConcurrent { atomicFile.createFileOutputStreamForWrite(false) } - } - - @Test - fun `case 3 when accessed concurrently with fix it will not trigger error`() { - val baseFile = createTargetFile() - val atomicFile = AtomicFile(baseFile) - executeConcurrent { atomicFile.createFileOutputStreamForWrite(true) } - } - - - private fun createTargetFile(): File { - return File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") - } - - private fun makeParentFolder() { - val parent = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234") - parent.mkdirs() - } - - private fun app(): Application { - return ApplicationProvider.getApplicationContext() - } -} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt deleted file mode 100644 index 19ee662..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileIntegrationTest.kt +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import android.app.Application -import androidx.test.core.app.ApplicationProvider -import com.google.common.truth.Truth -import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner -import java.io.File -import java.io.FileNotFoundException -import java.io.FileOutputStream -import java.nio.charset.StandardCharsets - -@RunWith(RobolectricTestRunner::class) -class AtomicFileIntegrationTest { - - @Test - fun `case 0 when value written will be persisted as an independent file`() { - val file = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") - val fileOutputStream = fileOutputStream(file) - fileOutputStream.write("123".toByteArray(StandardCharsets.UTF_8)) - fileOutputStream.close() - val atomicFile = AtomicFile(file) - val readFully = atomicFile.readFully() - Truth.assertThat(readFully).isEqualTo("123".toByteArray(StandardCharsets.UTF_8)) - } - - private fun fileOutputStream(file: File): FileOutputStream { - return try { - file.outputStream() - } catch (e: FileNotFoundException) { - val parentFile = file.parentFile ?: throw e - val failed = checkWithLock(parentFile) - if (failed) { - throw e - } else { - file.outputStream() - } - } - } - - @Synchronized - private fun checkWithLock(parentFile: File): Boolean { - return !parentFile.exists() && !parentFile.mkdirs() - } - - private fun app(): Application { - return ApplicationProvider.getApplicationContext() - } -} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt deleted file mode 100644 index cbdbdc4..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import net.jodah.concurrentunit.Waiter -import java.util.concurrent.Executors - - -/** - * A utility class to execute a given action concurrently. - * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. - * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. - */ -object ConcurrentTestUtil { - /** - * The maximum number of threads to be executed concurrently. - */ - private const val MAX_THREAD = 5 - - - /** - * Executes the given action concurrently. - * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. - * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. - */ - fun executeConcurrent(action: () -> Unit) { - val waiter = Waiter() - for (i in 0 until 1000) { - val service = Executors.newFixedThreadPool(MAX_THREAD) - for (j in 0 until 10) { - service.submit { - executeConcurrentInternally(action, waiter) - } - } - } - waiter.await(1000L * MAX_THREAD) - } - - private fun executeConcurrentInternally(action: () -> Unit, waiter: Waiter) { - try { - run(action) - } catch (e: Exception) { - waiter.fail(e) - } finally { - waiter.resume() - } - } - - -} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt b/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt deleted file mode 100644 index 995d10f..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreRepo.kt +++ /dev/null @@ -1,67 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import android.app.Application -import com.google.common.util.concurrent.ListenableFuture -import com.uber.simplestore.NamespaceConfig -import com.uber.simplestore.primitive.PrimitiveSimpleStore -import com.uber.simplestore.primitive.PrimitiveSimpleStoreFactory -import io.reactivex.Single - -internal class PrimitiveSimpleStoreRepo(app: Application) { - - private val dataStore = dataStore(app) - - fun writeSingle(counter: Long, key: String): Single { - return Single.fromFuture(listenableFuture(key, counter)) - } - - - private fun listenableFuture(key: String, counter: Long): ListenableFuture { - return dataStore.put(key, counter) - } - - fun writeString(value: String, key: String): Single { - return Single.fromFuture(dataStore.putString(key, value)) - } - - fun readSingle(key: String): Single { - return Single.fromFuture(dataStore.getLong(key)) - } - - /** No more read fallback as it is new API added. */ - fun readString(key: String): Single { - return Single.fromFuture(dataStore.getString(key)) - } - - - fun close() { - dataStore.close() - } - - companion object { - - private fun dataStore(app: Application): PrimitiveSimpleStore { - return PrimitiveSimpleStoreFactory.create( - AndroidDirectoryProvider(app), - "657b3cd7-f689-451b-aca0-628de60aa234", - NamespaceConfig.CRITICAL - ) - } - - } -} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt deleted file mode 100644 index 796c592..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/PrimitiveSimpleStoreTestUtil.kt +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - - -object PrimitiveSimpleStoreTestUtil { - - fun transformToLong(b: ByteArray): Long { - return b[0].toLong() and 0xFFL shl 56 or - (b[1].toLong() and 0xFFL shl 48) or - (b[2].toLong() and 0xFFL shl 40) or - (b[3].toLong() and 0xFFL shl 32) or - (b[4].toLong() and 0xFFL shl 24) or - (b[5].toLong() and 0xFFL shl 16) or - (b[6].toLong() and 0xFFL shl 8) or - (b[7].toLong() and 0xFFL) - } - -} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt deleted file mode 100644 index 7806f33..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreIntegrationTest.kt +++ /dev/null @@ -1,82 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import android.app.Application -import androidx.test.core.app.ApplicationProvider -import com.google.common.truth.Truth.assertThat -import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner -import java.io.File -import java.nio.charset.StandardCharsets - -@RunWith(RobolectricTestRunner::class) -class SimpleStoreIntegrationTest { - - @Test - fun `case 0 when value written will be persisted as an independent file`() { - generateFile() - assertFile1() - assertFile2() - } - - private fun generateFile() { - val repo = repo() - val test1 = repo.writeSingle(123, "file1").test() - val test2 = repo.writeString("456", "file2").test() - test1.assertValue(123) - test2.assertValue("456") - repo.close() - } - - private fun assertFile1() { - File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/file1").apply { - assertThat(exists()).isTrue() - val atomicFile = AtomicFile(this) - val readFully = atomicFile.readFully() - val bytes = readFully - val value = PrimitiveSimpleStoreTestUtil.transformToLong(bytes) - assertThat(value).isNotEqualTo("123") - assertThat(value).isEqualTo(123) - val repo = repo() - repo.readSingle("file1").test().assertValue(123) - repo.close() - } - } - - private fun assertFile2() { - File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/file2").apply { - assertThat(exists()).isTrue() - val atomicFile = AtomicFile(this) - val readFully = atomicFile.readFully() - val bytes = readFully - val value =String(bytes, StandardCharsets.UTF_16BE) - assertThat(value).isEqualTo("456") - val repo = repo() - repo.readString("file2").test().assertValue("456") - repo.close() - } - } - - private fun repo(): PrimitiveSimpleStoreRepo { - return PrimitiveSimpleStoreRepo(app()) - } - - private fun app(): Application { - return ApplicationProvider.getApplicationContext() - } -} From faf1391384535124aaaaeef381722fe4ee24ed4d Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Sat, 14 Sep 2024 22:28:33 -0700 Subject: [PATCH 21/28] Apply the minimum fix with test code --- gradle/dependencies.gradle | 3 +- simplestore/build.gradle | 4 + .../com/uber/simplestore/impl/AtomicFile.java | 54 +++++++++-- .../impl/AtomicFileConcurrentTest.kt | 89 +++++++++++++++++++ .../simplestore/impl/ConcurrentTestUtil.kt | 63 +++++++++++++ 5 files changed, 204 insertions(+), 9 deletions(-) create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 9a219c5..1160c98 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -57,7 +57,8 @@ def test = [ junitX : 'androidx.test.ext:junit:1.0.0', truth : 'com.google.truth:truth:1.1', truthX : 'androidx.test.ext:truth:1.5.0', - robolectric: 'org.robolectric:robolectric:4.4', + robolectric: 'org.robolectric:robolectric:4.5.1', + rxjava2: 'io.reactivex.rxjava2:rxjava:2.2.21', espresso : 'androidx.test.espresso:espresso-core:3.5.1', runner : 'androidx.test:runner:1.5.2', rules : 'androidx.test:rules:1.5.0', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index c81eda7..bb7e956 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -23,6 +23,9 @@ android { testOptions { execution 'ANDROIDX_TEST_ORCHESTRATOR' + unitTests { + includeAndroidResources = true + } } defaultConfig { @@ -47,6 +50,7 @@ dependencies { testImplementation deps.test.truthX testImplementation deps.test.concurrentunit testImplementation deps.test.robolectric + testImplementation deps.test.rxjava2 androidTestImplementation deps.test.runner androidTestImplementation deps.test.rules diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index 2273d7a..c596eeb 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -16,6 +16,7 @@ package com.uber.simplestore.impl; import android.util.Log; +import com.google.common.annotations.VisibleForTesting; import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -45,6 +46,8 @@ final class AtomicFile { private static final String LOG_TAG = "AtomicFile"; + private static final Object lock = new Object(); + private final File mBaseName; private final File mNewName; private final File mLegacyBackupName; @@ -89,20 +92,55 @@ public FileOutputStream startWrite() throws IOException { if (mLegacyBackupName.exists()) { rename(mLegacyBackupName, mBaseName); } + return createFileOutputStreamForWrite(true); + } + + private FileOutputStream createParentLegacy(FileNotFoundException e) throws IOException { + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw new IOException("Failed to create directory for " + mNewName); + } + try { + return new FileOutputStream(mNewName); + } catch (FileNotFoundException e2) { + throw new IOException("Failed to create new file " + mNewName, e2); + } + } + @VisibleForTesting + FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationException) + throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw new IOException("Failed to create directory for " + mNewName); - } - try { - return new FileOutputStream(mNewName); - } catch (FileNotFoundException e2) { - throw new IOException("Failed to create new file " + mNewName, e2); + return createParent(e, fixOnFileNotFoundationException); + } + } + + private FileOutputStream createParent( + FileNotFoundException rawError, boolean fixOnFileNotFoundationException) throws IOException { + if (fixOnFileNotFoundationException) { + return createParentWithFix(rawError); + } else { + return createParentLegacy(rawError); + } + } + + private FileOutputStream createParentWithFix(FileNotFoundException rawError) throws IOException { + File parent = mNewName.getParentFile(); + if (parent == null) { + throw rawError; + } + synchronized (lock) { + if (!parent.exists() && !parent.mkdirs()) { + throw rawError; } } + try { + return new FileOutputStream(mNewName); + } catch (FileNotFoundException e2) { + throw new IOException("Failed to create new file " + mNewName, e2); + } } /** diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt new file mode 100644 index 0000000..c3fb823 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt @@ -0,0 +1,89 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.uber.simplestore.impl.ConcurrentTestUtil.executeConcurrent +import org.junit.Ignore +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File +import java.io.FileNotFoundException + + +@RunWith(RobolectricTestRunner::class) +class AtomicFileConcurrentTest { + + @Test(expected = FileNotFoundException::class) + fun `case 0 when parent folder is absent will trigger FileNotFoundException`() { + //arrange + //makeParentFolder() + val targetFile = createTargetFile() + //act + val target = targetFile.outputStream() + //assert + assertThat(target).isNotNull() + } + + @Test + fun `case 1 when parent folder is present will not trigger FileNotFoundException`() { + //arrange + makeParentFolder() + val targetFile = createTargetFile() + //act + val target = targetFile.outputStream() + //assert + assertThat(target).isNotNull() + } + + @Ignore("This is failing due to CI flakiness. One could enable it locally to reproduce the issue.") + @Test(expected = AssertionError::class) + fun `case 2 when accessed concurrently without fix it will trigger error`() { + val baseFile = createTargetFile() + val atomicFile = AtomicFile(baseFile) + executeConcurrent { atomicFile.createFileOutputStreamForWrite(false) } + } + + @Test + fun `case 3 when accessed concurrently with fix it will not trigger error`() { + val baseFile = createTargetFile() + val atomicFile = AtomicFile(baseFile) + executeConcurrent { atomicFile.createFileOutputStreamForWrite(true) } + } + + + /** + * This represents a typical file that stores a value for a random key. + */ + private fun createTargetFile(): File { + return File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + } + + /** + * This presents a typical simple store folder scoped with under an `uuid`. + */ + private fun makeParentFolder() { + val parent = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234") + parent.mkdirs() + } + + private fun app(): Application { + return ApplicationProvider.getApplicationContext() + } +} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt new file mode 100644 index 0000000..cbdbdc4 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt @@ -0,0 +1,63 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import net.jodah.concurrentunit.Waiter +import java.util.concurrent.Executors + + +/** + * A utility class to execute a given action concurrently. + * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. + * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. + */ +object ConcurrentTestUtil { + /** + * The maximum number of threads to be executed concurrently. + */ + private const val MAX_THREAD = 5 + + + /** + * Executes the given action concurrently. + * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. + * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. + */ + fun executeConcurrent(action: () -> Unit) { + val waiter = Waiter() + for (i in 0 until 1000) { + val service = Executors.newFixedThreadPool(MAX_THREAD) + for (j in 0 until 10) { + service.submit { + executeConcurrentInternally(action, waiter) + } + } + } + waiter.await(1000L * MAX_THREAD) + } + + private fun executeConcurrentInternally(action: () -> Unit, waiter: Waiter) { + try { + run(action) + } catch (e: Exception) { + waiter.fail(e) + } finally { + waiter.resume() + } + } + + +} From 9446f543e046f8254d9aa2a352c160efe2eca63c Mon Sep 17 00:00:00 2001 From: TonyTang Date: Mon, 16 Sep 2024 08:53:50 -0700 Subject: [PATCH 22/28] Apply java.nio.file.Files.createDirectories for Android SDK 26 or later. (#21) --- .../com/uber/simplestore/impl/AtomicFile.java | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index c596eeb..c8a9f1d 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -22,6 +22,8 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; import org.jetbrains.annotations.Nullable; /* @@ -131,9 +133,10 @@ private FileOutputStream createParentWithFix(FileNotFoundException rawError) thr if (parent == null) { throw rawError; } + synchronized (lock) { - if (!parent.exists() && !parent.mkdirs()) { - throw rawError; + if (!parent.exists()) { + createParentInternal(rawError, parent); } } try { @@ -143,6 +146,22 @@ private FileOutputStream createParentWithFix(FileNotFoundException rawError) thr } } + private void createParentInternal(FileNotFoundException rawError, File parent) + throws IOException { + if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) { + Path parentPath = mNewName.toPath().getParent(); + if (parentPath != null && !Files.exists(parentPath)) { + Files.createDirectories(parentPath); + } else { + throw rawError; + } + } else { + if (!parent.mkdirs()) { + throw rawError; + } + } + } + /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From bf929c91f6345de96624a07be82497c24abd874f Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Mon, 16 Sep 2024 09:00:16 -0700 Subject: [PATCH 23/28] Revert "Apply java.nio.file.Files.createDirectories for Android SDK 26 or later. (#21)" This reverts commit 9446f543e046f8254d9aa2a352c160efe2eca63c. --- .../com/uber/simplestore/impl/AtomicFile.java | 23 ++----------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index c8a9f1d..c596eeb 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -22,8 +22,6 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; import org.jetbrains.annotations.Nullable; /* @@ -133,10 +131,9 @@ private FileOutputStream createParentWithFix(FileNotFoundException rawError) thr if (parent == null) { throw rawError; } - synchronized (lock) { - if (!parent.exists()) { - createParentInternal(rawError, parent); + if (!parent.exists() && !parent.mkdirs()) { + throw rawError; } } try { @@ -146,22 +143,6 @@ private FileOutputStream createParentWithFix(FileNotFoundException rawError) thr } } - private void createParentInternal(FileNotFoundException rawError, File parent) - throws IOException { - if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) { - Path parentPath = mNewName.toPath().getParent(); - if (parentPath != null && !Files.exists(parentPath)) { - Files.createDirectories(parentPath); - } else { - throw rawError; - } - } else { - if (!parent.mkdirs()) { - throw rawError; - } - } - } - /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From 790a1f28beaafc77aa3ea94abed96789f9fe1faf Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Mon, 16 Sep 2024 09:00:17 -0700 Subject: [PATCH 24/28] Revert "Apply the minimum fix with test code" This reverts commit faf1391384535124aaaaeef381722fe4ee24ed4d. --- gradle/dependencies.gradle | 3 +- simplestore/build.gradle | 4 - .../com/uber/simplestore/impl/AtomicFile.java | 54 ++--------- .../impl/AtomicFileConcurrentTest.kt | 89 ------------------- .../simplestore/impl/ConcurrentTestUtil.kt | 63 ------------- 5 files changed, 9 insertions(+), 204 deletions(-) delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt delete mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 1160c98..9a219c5 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -57,8 +57,7 @@ def test = [ junitX : 'androidx.test.ext:junit:1.0.0', truth : 'com.google.truth:truth:1.1', truthX : 'androidx.test.ext:truth:1.5.0', - robolectric: 'org.robolectric:robolectric:4.5.1', - rxjava2: 'io.reactivex.rxjava2:rxjava:2.2.21', + robolectric: 'org.robolectric:robolectric:4.4', espresso : 'androidx.test.espresso:espresso-core:3.5.1', runner : 'androidx.test:runner:1.5.2', rules : 'androidx.test:rules:1.5.0', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index bb7e956..c81eda7 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -23,9 +23,6 @@ android { testOptions { execution 'ANDROIDX_TEST_ORCHESTRATOR' - unitTests { - includeAndroidResources = true - } } defaultConfig { @@ -50,7 +47,6 @@ dependencies { testImplementation deps.test.truthX testImplementation deps.test.concurrentunit testImplementation deps.test.robolectric - testImplementation deps.test.rxjava2 androidTestImplementation deps.test.runner androidTestImplementation deps.test.rules diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index c596eeb..2273d7a 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -16,7 +16,6 @@ package com.uber.simplestore.impl; import android.util.Log; -import com.google.common.annotations.VisibleForTesting; import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -46,8 +45,6 @@ final class AtomicFile { private static final String LOG_TAG = "AtomicFile"; - private static final Object lock = new Object(); - private final File mBaseName; private final File mNewName; private final File mLegacyBackupName; @@ -92,54 +89,19 @@ public FileOutputStream startWrite() throws IOException { if (mLegacyBackupName.exists()) { rename(mLegacyBackupName, mBaseName); } - return createFileOutputStreamForWrite(true); - } - - private FileOutputStream createParentLegacy(FileNotFoundException e) throws IOException { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw new IOException("Failed to create directory for " + mNewName); - } - try { - return new FileOutputStream(mNewName); - } catch (FileNotFoundException e2) { - throw new IOException("Failed to create new file " + mNewName, e2); - } - } - @VisibleForTesting - FileOutputStream createFileOutputStreamForWrite(boolean fixOnFileNotFoundationException) - throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - return createParent(e, fixOnFileNotFoundationException); - } - } - - private FileOutputStream createParent( - FileNotFoundException rawError, boolean fixOnFileNotFoundationException) throws IOException { - if (fixOnFileNotFoundationException) { - return createParentWithFix(rawError); - } else { - return createParentLegacy(rawError); - } - } - - private FileOutputStream createParentWithFix(FileNotFoundException rawError) throws IOException { - File parent = mNewName.getParentFile(); - if (parent == null) { - throw rawError; - } - synchronized (lock) { - if (!parent.exists() && !parent.mkdirs()) { - throw rawError; + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw new IOException("Failed to create directory for " + mNewName); + } + try { + return new FileOutputStream(mNewName); + } catch (FileNotFoundException e2) { + throw new IOException("Failed to create new file " + mNewName, e2); } - } - try { - return new FileOutputStream(mNewName); - } catch (FileNotFoundException e2) { - throw new IOException("Failed to create new file " + mNewName, e2); } } diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt deleted file mode 100644 index c3fb823..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt +++ /dev/null @@ -1,89 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import android.app.Application -import androidx.test.core.app.ApplicationProvider -import com.google.common.truth.Truth.assertThat -import com.uber.simplestore.impl.ConcurrentTestUtil.executeConcurrent -import org.junit.Ignore -import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner -import java.io.File -import java.io.FileNotFoundException - - -@RunWith(RobolectricTestRunner::class) -class AtomicFileConcurrentTest { - - @Test(expected = FileNotFoundException::class) - fun `case 0 when parent folder is absent will trigger FileNotFoundException`() { - //arrange - //makeParentFolder() - val targetFile = createTargetFile() - //act - val target = targetFile.outputStream() - //assert - assertThat(target).isNotNull() - } - - @Test - fun `case 1 when parent folder is present will not trigger FileNotFoundException`() { - //arrange - makeParentFolder() - val targetFile = createTargetFile() - //act - val target = targetFile.outputStream() - //assert - assertThat(target).isNotNull() - } - - @Ignore("This is failing due to CI flakiness. One could enable it locally to reproduce the issue.") - @Test(expected = AssertionError::class) - fun `case 2 when accessed concurrently without fix it will trigger error`() { - val baseFile = createTargetFile() - val atomicFile = AtomicFile(baseFile) - executeConcurrent { atomicFile.createFileOutputStreamForWrite(false) } - } - - @Test - fun `case 3 when accessed concurrently with fix it will not trigger error`() { - val baseFile = createTargetFile() - val atomicFile = AtomicFile(baseFile) - executeConcurrent { atomicFile.createFileOutputStreamForWrite(true) } - } - - - /** - * This represents a typical file that stores a value for a random key. - */ - private fun createTargetFile(): File { - return File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") - } - - /** - * This presents a typical simple store folder scoped with under an `uuid`. - */ - private fun makeParentFolder() { - val parent = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234") - parent.mkdirs() - } - - private fun app(): Application { - return ApplicationProvider.getApplicationContext() - } -} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt deleted file mode 100644 index cbdbdc4..0000000 --- a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright (C) 2019. Uber Technologies - * - * 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 com.uber.simplestore.impl - -import net.jodah.concurrentunit.Waiter -import java.util.concurrent.Executors - - -/** - * A utility class to execute a given action concurrently. - * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. - * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. - */ -object ConcurrentTestUtil { - /** - * The maximum number of threads to be executed concurrently. - */ - private const val MAX_THREAD = 5 - - - /** - * Executes the given action concurrently. - * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. - * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. - */ - fun executeConcurrent(action: () -> Unit) { - val waiter = Waiter() - for (i in 0 until 1000) { - val service = Executors.newFixedThreadPool(MAX_THREAD) - for (j in 0 until 10) { - service.submit { - executeConcurrentInternally(action, waiter) - } - } - } - waiter.await(1000L * MAX_THREAD) - } - - private fun executeConcurrentInternally(action: () -> Unit, waiter: Waiter) { - try { - run(action) - } catch (e: Exception) { - waiter.fail(e) - } finally { - waiter.resume() - } - } - - -} From 5b77bd7752135f8f69889fffe0c758c215109cf2 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Mon, 16 Sep 2024 09:02:24 -0700 Subject: [PATCH 25/28] resolveParentFolder with Files.createDirectories without concurrent fix --- .../com/uber/simplestore/impl/AtomicFile.java | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index 2273d7a..4dd4c50 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -21,6 +21,9 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + import org.jetbrains.annotations.Nullable; /* @@ -93,10 +96,7 @@ public FileOutputStream startWrite() throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw new IOException("Failed to create directory for " + mNewName); - } + resolveParentFolder(e); try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e2) { @@ -105,6 +105,22 @@ public FileOutputStream startWrite() throws IOException { } } + private void resolveParentFolder(FileNotFoundException rawError) throws IOException { + if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) { + Path parentPath = mNewName.toPath().getParent(); + if (parentPath != null && !Files.exists(parentPath)) { + Files.createDirectories(parentPath); + } else { + throw rawError; + } + } else { + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw rawError; + } + } + } + /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From c2eae06bfa8235288ada095f11b534ff570b9250 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Mon, 16 Sep 2024 09:17:56 -0700 Subject: [PATCH 26/28] Revert "resolveParentFolder with Files.createDirectories without concurrent fix" This reverts commit 212c3865d8837ab417d8fd6880b8efb6c335bbfc. --- .../com/uber/simplestore/impl/AtomicFile.java | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index 4dd4c50..2273d7a 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -21,9 +21,6 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; - import org.jetbrains.annotations.Nullable; /* @@ -96,7 +93,10 @@ public FileOutputStream startWrite() throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - resolveParentFolder(e); + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw new IOException("Failed to create directory for " + mNewName); + } try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e2) { @@ -105,22 +105,6 @@ public FileOutputStream startWrite() throws IOException { } } - private void resolveParentFolder(FileNotFoundException rawError) throws IOException { - if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) { - Path parentPath = mNewName.toPath().getParent(); - if (parentPath != null && !Files.exists(parentPath)) { - Files.createDirectories(parentPath); - } else { - throw rawError; - } - } else { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw rawError; - } - } - } - /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From f9100b18148537e833cb64b11e43f7667f838bfa Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Mon, 16 Sep 2024 09:19:29 -0700 Subject: [PATCH 27/28] resolveParentFolder with Files.createDirectories without concurrent fix --- .../com/uber/simplestore/impl/AtomicFile.java | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java index 2273d7a..4dd4c50 100644 --- a/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java +++ b/simplestore/src/main/java/com/uber/simplestore/impl/AtomicFile.java @@ -21,6 +21,9 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + import org.jetbrains.annotations.Nullable; /* @@ -93,10 +96,7 @@ public FileOutputStream startWrite() throws IOException { try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e) { - File parent = mNewName.getParentFile(); - if (!parent.mkdirs()) { - throw new IOException("Failed to create directory for " + mNewName); - } + resolveParentFolder(e); try { return new FileOutputStream(mNewName); } catch (FileNotFoundException e2) { @@ -105,6 +105,22 @@ public FileOutputStream startWrite() throws IOException { } } + private void resolveParentFolder(FileNotFoundException rawError) throws IOException { + if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) { + Path parentPath = mNewName.toPath().getParent(); + if (parentPath != null && !Files.exists(parentPath)) { + Files.createDirectories(parentPath); + } else { + throw rawError; + } + } else { + File parent = mNewName.getParentFile(); + if (!parent.mkdirs()) { + throw rawError; + } + } + } + /** * Call when you have successfully finished writing to the stream returned by {@link * #startWrite()}. This will close, sync, and commit the new data. The next attempt to read the From 033d38490c328c50e59593030b1d25ef39f62b25 Mon Sep 17 00:00:00 2001 From: Tony Tang Date: Mon, 16 Sep 2024 09:25:09 -0700 Subject: [PATCH 28/28] Add boilerplate test code --- gradle/dependencies.gradle | 2 +- simplestore/build.gradle | 3 + .../impl/AtomicFileConcurrentTest.kt | 74 +++++++++++++++++++ .../simplestore/impl/ConcurrentTestUtil.kt | 63 ++++++++++++++++ 4 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt create mode 100644 simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 9a219c5..2aac369 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -57,7 +57,7 @@ def test = [ junitX : 'androidx.test.ext:junit:1.0.0', truth : 'com.google.truth:truth:1.1', truthX : 'androidx.test.ext:truth:1.5.0', - robolectric: 'org.robolectric:robolectric:4.4', + robolectric: 'org.robolectric:robolectric:4.5.1', espresso : 'androidx.test.espresso:espresso-core:3.5.1', runner : 'androidx.test:runner:1.5.2', rules : 'androidx.test:rules:1.5.0', diff --git a/simplestore/build.gradle b/simplestore/build.gradle index c81eda7..19428fb 100644 --- a/simplestore/build.gradle +++ b/simplestore/build.gradle @@ -23,6 +23,9 @@ android { testOptions { execution 'ANDROIDX_TEST_ORCHESTRATOR' + unitTests { + includeAndroidResources = true + } } defaultConfig { diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt new file mode 100644 index 0000000..42245ff --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/AtomicFileConcurrentTest.kt @@ -0,0 +1,74 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.uber.simplestore.impl.ConcurrentTestUtil.executeConcurrent +import org.junit.Ignore +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File +import java.io.FileNotFoundException + + +@RunWith(RobolectricTestRunner::class) +class AtomicFileConcurrentTest { + + @Test(expected = FileNotFoundException::class) + fun `case 0 when parent folder is absent will trigger FileNotFoundException`() { + //arrange + //makeParentFolder() + val targetFile = createTargetFile() + //act + val target = targetFile.outputStream() + //assert + assertThat(target).isNotNull() + } + + @Test + fun `case 1 when parent folder is present will not trigger FileNotFoundException`() { + //arrange + makeParentFolder() + val targetFile = createTargetFile() + //act + val target = targetFile.outputStream() + //assert + assertThat(target).isNotNull() + } + + + /** + * This represents a typical file that stores a value for a random key. + */ + private fun createTargetFile(): File { + return File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234/random_key") + } + + /** + * This presents a typical simple store folder scoped with under an `uuid`. + */ + private fun makeParentFolder() { + val parent = File(app().dataDir, "files/simplestore/657b3cd7-f689-451b-aca0-628de60aa234") + parent.mkdirs() + } + + private fun app(): Application { + return ApplicationProvider.getApplicationContext() + } +} diff --git a/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt new file mode 100644 index 0000000..cbdbdc4 --- /dev/null +++ b/simplestore/src/test/java/com/uber/simplestore/impl/ConcurrentTestUtil.kt @@ -0,0 +1,63 @@ +/* + * Copyright (C) 2019. Uber Technologies + * + * 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 com.uber.simplestore.impl + +import net.jodah.concurrentunit.Waiter +import java.util.concurrent.Executors + + +/** + * A utility class to execute a given action concurrently. + * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. + * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. + */ +object ConcurrentTestUtil { + /** + * The maximum number of threads to be executed concurrently. + */ + private const val MAX_THREAD = 5 + + + /** + * Executes the given action concurrently. + * It will trigger [Waiter.fail] if the action throws an exception during the whole concurrent execution process. + * It will trigger [Waiter.resume] if the action is concluded successfully for the whole concurrent execution. + */ + fun executeConcurrent(action: () -> Unit) { + val waiter = Waiter() + for (i in 0 until 1000) { + val service = Executors.newFixedThreadPool(MAX_THREAD) + for (j in 0 until 10) { + service.submit { + executeConcurrentInternally(action, waiter) + } + } + } + waiter.await(1000L * MAX_THREAD) + } + + private fun executeConcurrentInternally(action: () -> Unit, waiter: Waiter) { + try { + run(action) + } catch (e: Exception) { + waiter.fail(e) + } finally { + waiter.resume() + } + } + + +}