Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix corner cases in tests discovered after running on Github Actions #1579

Merged
merged 3 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmarks/settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
// For CI buils, the packages are expected to have
// been built and deployed to a local filesystem
// maven repo.
if (System.getenv("JENKINS_HOME") == null) {
if (System.getenv("JENKINS_HOME") == null && System.getenv("CI") == null) {
includeBuild("../packages")
}

Expand Down
2 changes: 1 addition & 1 deletion buildSrc/src/main/kotlin/Config.kt
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ private fun findHostOs(): OperatingSystem {
val HOST_OS: OperatingSystem = findHostOs()

object Realm {
val ciBuild = (System.getenv("JENKINS_HOME") != null)
val ciBuild = (System.getenv("JENKINS_HOME") != null || System.getenv("CI") != null)
const val version = "1.13.0-SNAPSHOT"
const val group = "io.realm.kotlin"
const val projectUrl = "https://realm.io"
Expand Down
2 changes: 1 addition & 1 deletion examples/kmm-sample/settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// been built and deployed to a local filesystem
// maven repo. We cannot reference `Realm.ciBuild`
// from buildSrc here.
if (System.getenv("JENKINS_HOME") == null) {
if (System.getenv("JENKINS_HOME") == null && System.getenv("CI") == null) {
includeBuild("../../packages")
}

Expand Down
2 changes: 1 addition & 1 deletion examples/realm-java-compatibility/settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// been built and deployed to a local filesystem
// maven repo. We cannot reference `Realm.ciBuild`
// from buildSrc here.
if (System.getenv("JENKINS_HOME") == null) {
if (System.getenv("JENKINS_HOME") == null && System.getenv("CI") == null) {
includeBuild("../../packages")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,32 +300,30 @@ public interface Configuration {
} as S

/**
* Dispatcher used to run background writes to the Realm.
* Dispatcher on which Realm notifications are run. It is possible to listen for changes to
* Realm objects from any thread, but the underlying logic will run on this dispatcher
* before any changes are returned to the caller thread.
cmelchior marked this conversation as resolved.
Show resolved Hide resolved
*
* Defaults to a single threaded dispatcher started when the configuration is built.
*
* NOTE On Android the dispatcher's thread must have an initialized
* [Looper](https://developer.android.com/reference/android/os/Looper#prepare()).
*
* @param dispatcher dispatcher on which writes are run. It is required to be backed by a
* single thread only.
* @param dispatcher dispatcher on which notifications are run. It is required to be backed
* by a single thread only.
*/
internal fun notificationDispatcher(dispatcher: CoroutineDispatcher) = apply {
this.notificationDispatcher = dispatcher
} as S

/**
* Dispatcher on which Realm notifications are run. It is possible to listen for changes to
* Realm objects from any thread, but the underlying logic will run on this dispatcher
* before any changes are returned to the caller thread.
*
* Defaults to a single threaded dispatcher started when the configuration is built.
* Dispatcher used to run background writes to the Realm. *
*
* NOTE On Android the dispatcher's thread must have an initialized
* [Looper](https://developer.android.com/reference/android/os/Looper#prepare()).
*
* @param dispatcher Dispatcher on which notifications are run. It is required to be backed
* by a single thread only.
* @param dispatcher dispatcher on which writes are run. It is required to be backed by a
* single thread only.
*/
internal fun writeDispatcher(dispatcher: CoroutineDispatcher) = apply {
this.writeDispatcher = dispatcher
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import io.realm.kotlin.internal.interop.RealmInterop
import io.realm.kotlin.internal.interop.RealmSchemaPointer
import io.realm.kotlin.internal.interop.SchemaMode
import io.realm.kotlin.internal.interop.use
import io.realm.kotlin.internal.platform.PATH_SEPARATOR
import io.realm.kotlin.internal.platform.appFilesDirectory
import io.realm.kotlin.internal.platform.prepareRealmFilePath
import io.realm.kotlin.internal.platform.realmObjectCompanionOrThrow
Expand Down Expand Up @@ -255,8 +256,8 @@ public open class ConfigurationImpl(
private fun normalizePath(directoryPath: String, fileName: String): String {
var dir = directoryPath.ifEmpty { appFilesDirectory() }
// If dir is a relative path, replace with full path for easier debugging
if (dir.startsWith("./")) {
dir = dir.replaceFirst("./", "${appFilesDirectory()}/")
if (dir.startsWith(".$PATH_SEPARATOR")) {
dir = dir.replaceFirst(".$PATH_SEPARATOR", "${appFilesDirectory()}$PATH_SEPARATOR")
}
return prepareRealmFilePath(dir, fileName)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ class Decimal128Tests {

@AfterTest
fun tearDown() {
if (this::realm.isInitialized) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ class EmbeddedRealmObjectTests {

@AfterTest
fun tearDown() {
if (this::realm.isInitialized) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ class FqNameImportTests {

@AfterTest
fun tearDown() {
if (this::realm.isInitialized) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ class ImportTests {

@AfterTest
fun tearDown() {
if (this::realm.isInitialized) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ class LinkTests {

@AfterTest
fun tearDown() {
if (this::realm.isInitialized) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ class NullabilityTests {

@AfterTest
fun tearDown() {
if (this::realm.isInitialized) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import io.realm.kotlin.notifications.UpdatedResults
import io.realm.kotlin.test.platform.PlatformUtils
import io.realm.kotlin.types.RealmList
import io.realm.kotlin.types.RealmObject
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CloseableCoroutineDispatcher
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.async
import kotlinx.coroutines.cancel
Expand All @@ -57,7 +57,7 @@ import kotlin.test.Test
*/
class ReadMeTests {
private lateinit var scope: CoroutineScope
private lateinit var context: CoroutineDispatcher
private lateinit var context: CloseableCoroutineDispatcher
lateinit var tmpDir: String
lateinit var realm: Realm

Expand All @@ -79,6 +79,7 @@ class ReadMeTests {
scope.cancel()
context.cancel()
realm.close()
context.close()
PlatformUtils.deleteTempDir(tmpDir)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class RealmConfigurationTests {

val configFromBuilderWithCurrentDir: RealmConfiguration =
RealmConfiguration.Builder(schema = setOf(Sample::class))
.directory("./my_dir")
.directory(pathOf(".", "my_dir"))
.name("foo.realm")
.build()
assertEquals(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ class RealmInMemoryTests {

@AfterTest
fun tearDown() {
PlatformUtils.deleteTempDir(tmpDir)
if (this::realm.isInitialized && !realm.isClosed()) {
realm.close()
}
PlatformUtils.deleteTempDir(tmpDir)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class RealmMigrationTests {
assertNull(newSchema["SchemaVariations"])
}
}
)
).close()
}

// TODO Test all schema modifications (theoretically test core behavior, so postponed for now)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ class RealmNotificationsTests : FlowableTests {

runBlocking {
val listener = async {
withTimeout(10.seconds) {
withTimeout(30.seconds) {
assertFailsWith<CancellationException> {
flow.collect {
delay(1000.milliseconds)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,9 @@ class SystemNotificationTests {
writer1.write { copyToRealm(Sample()) }
writer2.write { copyToRealm(Sample()) }
}
writer1.close()
writer2.close()
liveRealmContext.close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually green on CI? I tried to clean this up as part of another PR but ended up splitting into this separate issue #1563

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe closing the writers individually first make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is working on GHA.

baseRealm.close()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@

package io.realm.kotlin.test.platform

import java.io.File
import java.nio.file.Files
import java.nio.file.Path
import java.nio.file.Paths
import java.nio.file.attribute.PosixFilePermission
import java.util.stream.Collectors
import kotlin.io.path.absolutePathString
import kotlin.time.Duration

Expand All @@ -40,7 +41,22 @@ actual object PlatformUtils {
}

actual fun deleteTempDir(path: String) {
File(path).deleteRecursively()
val rootPath: Path = Paths.get(path)
val pathsToDelete: List<Path> =
Files.walk(rootPath).sorted(Comparator.reverseOrder()).collect(Collectors.toList())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the sort needed to delete files first?

for (p in pathsToDelete) {
try {
Files.deleteIfExists(p)
} catch (e: java.nio.file.FileSystemException) {
// Sometimes (on Windows) we need the give a GC a chance to run and close all native pointers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

// before we can delete the Realm, otherwise delete will fail with " The process cannot access the
// file because it is being used by another process".
//
// We try to trigger the GC once then retry the delete.
triggerGC()
Files.deleteIfExists(p)
}
}
}

actual fun sleep(duration: Duration) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,13 @@ class RealmTests {
// Closing a Realm should also cleanup our default (internal) dispatchers.
// The core notifier and the finalizer thread will never be closed.
val expectedThreadCount = initialThreads.size + 1 /* core-notifier */ + if (finalizerRunning) 0 else 1
var counter = 5 // Wait 5 seconds for threads to settle
while (totalThreadCount() != expectedThreadCount && counter > 0) {
var counter = 10 // Wait 5 seconds for threads to settle
cmelchior marked this conversation as resolved.
Show resolved Hide resolved
while (newThreads().any { !it.isDaemon } && counter > 0) {
delay(1000)
counter--
}
assertEquals(expectedThreadCount, totalThreadCount(), "Unexpected thread count after closing realm: ${newThreads()}")
val totalThreadCount = totalThreadCount()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of debugging, we should print the stack trace of all non-daemon thread to find where they're are blocking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but it would massively expand the output and so far all the cases we had for this, it was enough to get the name of the thread to figure out what was going on. So for now I think I would prefer to keep as as is.

assertTrue(totalThreadCount <= expectedThreadCount, "Unexpected thread count after closing realm: $expectedThreadCount <= $totalThreadCount. New threads: ${newThreads()}. Threads: ${threadTrace()}")

// Verify that all remaining threads are daemon threads, so that we don't keep the JVM alive
newThreads().filter { !it.isDaemon }.let {
Expand Down Expand Up @@ -122,4 +123,18 @@ class RealmTests {
Unit
}
}

private fun threadTrace(): String {
val sb = StringBuilder()
sb.appendLine("--------------------------------")
val stack = Thread.getAllStackTraces()
stack.keys
.sortedBy { it.name }
.forEach { t: Thread ->
sb.appendLine("${t.name} - Is Daemon ${t.isDaemon} - Is Alive ${t.isAlive}")
}
sb.appendLine("All threads: ${stack.keys.size}")
sb.appendLine("Active threads: ${Thread.activeCount()}")
return sb.toString()
}
}