From 330a06ca691fc8392f441cf9c746e5d5205e1d63 Mon Sep 17 00:00:00 2001 From: clementetb Date: Fri, 6 Oct 2023 09:59:53 +0200 Subject: [PATCH] Fix JVM memory leaks (#1526) --- CHANGELOG.md | 1 + .../kotlin/internal/interop/MemAllocator.kt | 12 +-- .../kotlin/internal/interop/RealmInterop.kt | 85 ++++++++++++++----- .../internal/interop/RealmValueAllocator.kt | 66 ++++++++------ .../src/main/jni/realm_api_helpers.cpp | 24 ++++-- .../src/main/jni/realm_api_helpers.h | 6 ++ 6 files changed, 137 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7621882b9c..fb960338d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed * `Realm.getNumberOfActiveVersions` now returns the actual number of active versions. (Core issue [#6960](https://github.com/realm/realm-core/pull/6960)) +* Fix memory leaks on the JVM platform, see PR for more information. (Issue [#1526](https://github.com/realm/realm-kotlin/pull/1526)) ### Compatibility * File format: Generates Realms with file format v23. diff --git a/packages/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/MemAllocator.kt b/packages/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/MemAllocator.kt index 52f1d89d71..7825ea7f30 100644 --- a/packages/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/MemAllocator.kt +++ b/packages/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/MemAllocator.kt @@ -93,12 +93,6 @@ interface MemAllocator { * Instantiates a [RealmValue] representing a `realm_value_t` of type `RLM_TYPE_LINK`. */ fun realmObjectTransport(value: RealmObjectInterop?): RealmValue - - /** - * Instantiates a [RealmQueryArgumentList] representing a `realm_query_arg_t` that describe and - * references the incoming [RealmValueList] arguments. - */ - fun queryArgsOf(queryArgs: List): RealmQueryArgumentList } /** @@ -118,6 +112,12 @@ interface MemTrackingAllocator : MemAllocator { */ fun byteArrayTransport(value: ByteArray?): RealmValue + /** + * Instantiates a [RealmQueryArgumentList] representing a `realm_query_arg_t` that describe and + * references the incoming [RealmValueList] arguments. + */ + fun queryArgsOf(queryArgs: List): RealmQueryArgumentList + /** * Frees resources linked to this allocator. See implementations for more details. */ diff --git a/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt b/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt index c2b2a07d8f..14674de642 100644 --- a/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt +++ b/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt @@ -17,6 +17,7 @@ package io.realm.kotlin.internal.interop import io.realm.kotlin.internal.interop.Constants.ENCRYPTION_KEY_LENGTH +import io.realm.kotlin.internal.interop.RealmInterop.cptr import io.realm.kotlin.internal.interop.sync.ApiKeyWrapper import io.realm.kotlin.internal.interop.sync.AuthProvider import io.realm.kotlin.internal.interop.sync.CoreConnectionState @@ -117,7 +118,35 @@ actual object RealmInterop { realmc.classArray_setitem(cclasses, i, cclass) realmc.propertyArrayArray_setitem(cproperties, i, classProperties) } - return LongPointerWrapper(realmc.realm_schema_new(cclasses, count.toLong(), cproperties)) + try { + return LongPointerWrapper(realmc.realm_schema_new(cclasses, count.toLong(), cproperties)) + } finally { + // Clean up classes + for (classIndex in 0 until count) { + val classInfo = realmc.classArray_getitem(cclasses, classIndex) + + // Clean properties + val propertyArray = realmc.propertyArrayArray_getitem(cproperties, classIndex) + + val propertyCount = classInfo.getNum_properties() + classInfo.getNum_computed_properties() + for (propertyIndex in 0 until propertyCount) { + val property = realmc.propertyArray_getitem(propertyArray, propertyIndex.toInt()) + + realmc.realm_property_info_t_cleanup(property) + property.delete() + } + + realmc.delete_propertyArray(propertyArray) + + // end clean properties + + realmc.realm_class_info_t_cleanup(classInfo) + classInfo.delete() + } + + realmc.delete_propertyArrayArray(cproperties) + realmc.delete_classArray(cclasses) + } } actual fun realm_config_new(): RealmConfigurationPointer { @@ -316,23 +345,27 @@ actual object RealmInterop { val properties = realmc.new_propertyArray(max.toInt()) val outCount = longArrayOf(0) realmc.realm_get_class_properties(realm.cptr(), classKey.key, properties, max, outCount) - return if (outCount[0] > 0) { - (0 until outCount[0]).map { i -> - with(realmc.propertyArray_getitem(properties, i.toInt())) { - PropertyInfo( - name, - public_name, - PropertyType.from(type), - CollectionType.from(collection_type), - link_target, - link_origin_property_name, - PropertyKey(key), - flags - ) + try { + return if (outCount[0] > 0) { + (0 until outCount[0]).map { i -> + with(realmc.propertyArray_getitem(properties, i.toInt())) { + PropertyInfo( + name, + public_name, + PropertyType.from(type), + CollectionType.from(collection_type), + link_target, + link_origin_property_name, + PropertyKey(key), + flags + ) + } } + } else { + emptyList() } - } else { - emptyList() + } finally { + realmc.delete_propertyArray(properties) } } @@ -922,6 +955,8 @@ actual object RealmInterop { builder.initIndicesArray(builder::modificationIndices, modificationIndices) builder.initIndicesArray(builder::modificationIndicesAfter, modificationIndicesAfter) builder.movesCount = movesCount[0].toInt() + + realmc.delete_collectionMoveArray(moves) } actual fun realm_collection_changes_get_ranges( @@ -969,6 +1004,12 @@ actual object RealmInterop { builder.initRangesArray(builder::insertionRanges, insertionRanges, insertRangesCount[0]) builder.initRangesArray(builder::modificationRanges, modificationRanges, modificationRangesCount[0]) builder.initRangesArray(builder::modificationRangesAfter, modificationRangesAfter, modificationRangesCount[0]) + + realmc.delete_indexRangeArray(insertionRanges) + realmc.delete_indexRangeArray(modificationRanges) + realmc.delete_indexRangeArray(modificationRangesAfter) + realmc.delete_indexRangeArray(deletionRanges) + realmc.delete_collectionMoveArray(moves) } actual fun realm_dictionary_get_changes( @@ -1107,6 +1148,8 @@ actual object RealmInterop { } } else { emptyList() + }.also { + realmc.delete_identityArray(keys) } } @@ -1609,7 +1652,7 @@ actual object RealmInterop { realm: RealmPointer, classKey: ClassKey, query: String, - args: RealmQueryArgumentList + args: RealmQueryArgumentList, ): RealmQueryPointer { return LongPointerWrapper( realmc.realm_query_parse( @@ -1625,7 +1668,7 @@ actual object RealmInterop { actual fun realm_query_parse_for_results( results: RealmResultsPointer, query: String, - args: RealmQueryArgumentList + args: RealmQueryArgumentList, ): RealmQueryPointer { return LongPointerWrapper( realmc.realm_query_parse_for_results( @@ -1640,7 +1683,7 @@ actual object RealmInterop { actual fun realm_query_parse_for_list( list: RealmListPointer, query: String, - args: RealmQueryArgumentList + args: RealmQueryArgumentList, ): RealmQueryPointer { return LongPointerWrapper( realmc.realm_query_parse_for_list( @@ -1655,7 +1698,7 @@ actual object RealmInterop { actual fun realm_query_parse_for_set( set: RealmSetPointer, query: String, - args: RealmQueryArgumentList + args: RealmQueryArgumentList, ): RealmQueryPointer { return LongPointerWrapper( realmc.realm_query_parse_for_set( @@ -1693,7 +1736,7 @@ actual object RealmInterop { actual fun realm_query_append_query( query: RealmQueryPointer, filter: String, - args: RealmQueryArgumentList + args: RealmQueryArgumentList, ): RealmQueryPointer { return LongPointerWrapper( realmc.realm_query_append_query(query.cptr(), filter, args.size, args.head) diff --git a/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmValueAllocator.kt b/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmValueAllocator.kt index 9999c7dfee..72331fd1db 100644 --- a/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmValueAllocator.kt +++ b/packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmValueAllocator.kt @@ -93,28 +93,6 @@ object JvmMemAllocator : MemAllocator { link = realmc.realm_object_as_link(it.objectPointer.cptr()) } - override fun queryArgsOf(queryArgs: List): RealmQueryArgumentList { - val cArgs = realmc.new_queryArgArray(queryArgs.size) - queryArgs.mapIndexed { index, arg -> - val queryArg = realm_query_arg_t().apply { - when (arg) { - is RealmQueryListArgument -> { - nb_args = arg.arguments.size.toLong() - is_list = true - this.arg = arg.arguments.head - } - is RealmQuerySingleArgument -> { - nb_args = 1 - is_list = false - this.arg = arg.argument.value - } - } - } - realmc.queryArgArray_setitem(cArgs, index, queryArg) - } - return RealmQueryArgumentList(queryArgs.size.toLong(), cArgs) - } - private inline fun createTransport( value: T?, type: Int, @@ -151,6 +129,32 @@ class JvmMemTrackingAllocator : MemAllocator by JvmMemAllocator, MemTrackingAllo } } + override fun queryArgsOf(queryArgs: List): RealmQueryArgumentList { + val cArgs = realmc.new_queryArgArray(queryArgs.size) + queryArgs.mapIndexed { index, arg -> + val queryArg = realm_query_arg_t().apply { + when (arg) { + is RealmQueryListArgument -> { + nb_args = arg.arguments.size.toLong() + is_list = true + this.arg = arg.arguments.head + + scope.manageQueryListArgument(arg) + } + is RealmQuerySingleArgument -> { + nb_args = 1 + is_list = false + this.arg = arg.argument.value + } + } + } + realmc.queryArgArray_setitem(cArgs, index, queryArg) + } + return RealmQueryArgumentList(queryArgs.size.toLong(), cArgs).also { + scope.manageQueryArgumentList(it) + } + } + /** * Frees resources linked to this allocator's [scope], more specifically strings and binary * buffers. See [MemScope.free] for more details. @@ -180,16 +184,28 @@ class JvmMemTrackingAllocator : MemAllocator by JvmMemAllocator, MemTrackingAllo * copied out afterwards). */ class MemScope { - val values: MutableSet = mutableSetOf() + val values: MutableSet = mutableSetOf() fun manageRealmValue(value: RealmValueT): RealmValueT { values.add(value) return value } + fun manageQueryArgumentList(value: RealmQueryArgumentList): RealmQueryArgumentList = value.also { + values.add(value) + } + + fun manageQueryListArgument(value: RealmQueryListArgument): RealmQueryListArgument = value.also { + values.add(value) + } + fun free() { - values.map { - realmc.realm_value_t_cleanup(it) + values.forEach { + when (it) { + is RealmValueT -> realmc.realm_value_t_cleanup(it) + is RealmQueryArgumentList -> realmc.delete_queryArgArray(it.head) + is RealmQueryListArgument -> realmc.delete_valueArray(it.arguments.head) + } } } } diff --git a/packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp b/packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp index 392e1a7797..1db4de4d21 100644 --- a/packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp +++ b/packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp @@ -721,10 +721,10 @@ jobject convert_to_jvm_sync_error(JNIEnv* jenv, const realm_sync_error_t& error) jboolean is_unrecognized_by_client = error.is_unrecognized_by_client; jboolean is_client_reset_requested = error.is_client_reset_requested; - auto user_info_map = new std::map(); + auto user_info_map = std::map(); for (int i = 0; i < error.user_info_length; i++) { realm_sync_error_user_info_t user_info = error.user_info_map[i]; - user_info_map->insert(std::make_pair(user_info.key, user_info.value)); + user_info_map.insert(std::make_pair(user_info.key, user_info.value)); } static JavaMethod core_compensating_write_info_constructor( @@ -766,16 +766,16 @@ jobject convert_to_jvm_sync_error(JNIEnv* jenv, const realm_sync_error_t& error) // mark the file for deletion. Having 'original path' in the user_info_map is a side effect of // using the same code for client reset. if (error.user_info_length > 0) { - auto end_it = user_info_map->end(); + auto end_it = user_info_map.end(); - auto original_it = user_info_map->find(error.c_original_file_path_key); + auto original_it = user_info_map.find(error.c_original_file_path_key); if (end_it != original_it) { auto original_file_path = original_it->second; joriginal_file_path = to_jstring(jenv, original_file_path); } // Sync errors may not have the path to the recovery file unless a Client Reset is requested - auto recovery_it = user_info_map->find(error.c_recovery_file_path_key); + auto recovery_it = user_info_map.find(error.c_recovery_file_path_key); if (error.is_client_reset_requested && (end_it != recovery_it)) { auto recovery_file_path = recovery_it->second; jrecovery_file_path = to_jstring(jenv, recovery_file_path); @@ -1053,3 +1053,17 @@ realm_scheduler_t* realm_create_generic_scheduler() { return new realm_scheduler_t { realm::util::Scheduler::make_dummy() }; } + +void +realm_property_info_t_cleanup(realm_property_info_t* value) { + delete[] value->link_origin_property_name; + delete[] value->link_target; + delete[] value->name; + delete[] value->public_name; +} + +void +realm_class_info_t_cleanup(realm_class_info_t * value) { + delete[] value->primary_key; + delete[] value->name; +} diff --git a/packages/jni-swig-stub/src/main/jni/realm_api_helpers.h b/packages/jni-swig-stub/src/main/jni/realm_api_helpers.h index 210202a6c9..022bca21a7 100644 --- a/packages/jni-swig-stub/src/main/jni/realm_api_helpers.h +++ b/packages/jni-swig-stub/src/main/jni/realm_api_helpers.h @@ -137,4 +137,10 @@ realm_sync_thread_error(realm_userdata_t userdata, const char* error); realm_scheduler_t* realm_create_generic_scheduler(); +void +realm_property_info_t_cleanup(realm_property_info_t* value); + +void +realm_class_info_t_cleanup(realm_class_info_t * value); + #endif //TEST_REALM_API_HELPERS_H