From 1562be55751a4c6ddb2af508ee29af07363193a2 Mon Sep 17 00:00:00 2001 From: Eugene R Date: Mon, 15 Jul 2024 12:48:35 +0300 Subject: [PATCH] FMWK-496 Resolve potential memory leak related to object references (#165) --- .../aerospike/mapper/tools/AeroMapper.java | 20 +++--- .../mapper/tools/LoadedObjectResolver.java | 21 ++++--- .../mapper/tools/ReactiveAeroMapper.java | 47 +++++++------- .../mapper/tools/ThreadLocalKeySaver.java | 18 ++++-- .../reactive/ReactiveRecursiveObjectTest.java | 62 +++++++++++++++++++ 5 files changed, 121 insertions(+), 47 deletions(-) create mode 100644 src/test/java/com/aerospike/mapper/reactive/ReactiveRecursiveObjectTest.java diff --git a/src/main/java/com/aerospike/mapper/tools/AeroMapper.java b/src/main/java/com/aerospike/mapper/tools/AeroMapper.java index 5c92d0a..975c7cc 100644 --- a/src/main/java/com/aerospike/mapper/tools/AeroMapper.java +++ b/src/main/java/com/aerospike/mapper/tools/AeroMapper.java @@ -1,13 +1,5 @@ package com.aerospike.mapper.tools; -import java.lang.reflect.Array; -import java.util.ArrayList; -import java.util.List; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Function; - -import javax.validation.constraints.NotNull; - import com.aerospike.client.AerospikeException; import com.aerospike.client.AerospikeException.ScanTerminated; import com.aerospike.client.Bin; @@ -29,7 +21,13 @@ import com.aerospike.mapper.tools.converters.MappingConverter; import com.aerospike.mapper.tools.utils.MapperUtils; import com.aerospike.mapper.tools.virtuallist.VirtualList; -import reactor.core.publisher.Mono; + +import javax.validation.constraints.NotNull; +import java.lang.reflect.Array; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Function; public class AeroMapper implements IAeroMapper { @@ -42,7 +40,7 @@ private AeroMapper(@NotNull IAerospikeClient client) { } /** - * Create a new Builder to instantiate the AeroMapper. + * Create a new Builder to instantiate the AeroMapper. * @author tfaulkes * */ @@ -233,12 +231,10 @@ private T read(Policy readPolicy, @NotNull Class clazz, @NotNull Key key, } else { try { ThreadLocalKeySaver.save(key); - LoadedObjectResolver.begin(); return mappingConverter.convertToObject(clazz, key, record, entry, resolveDependencies); } catch (ReflectiveOperationException e) { throw new AerospikeException(e); } finally { - LoadedObjectResolver.end(); ThreadLocalKeySaver.clear(); } } diff --git a/src/main/java/com/aerospike/mapper/tools/LoadedObjectResolver.java b/src/main/java/com/aerospike/mapper/tools/LoadedObjectResolver.java index 7fc8dbb..5a46bc3 100644 --- a/src/main/java/com/aerospike/mapper/tools/LoadedObjectResolver.java +++ b/src/main/java/com/aerospike/mapper/tools/LoadedObjectResolver.java @@ -1,19 +1,17 @@ package com.aerospike.mapper.tools; +import com.aerospike.client.Key; + import java.util.HashMap; import java.util.Map; -import com.aerospike.client.Key; - public class LoadedObjectResolver { - private static class LoadedObjectMap { - private int referenceCount = 0; - private final Map objectMap = new HashMap<>(); - } - private static final ThreadLocal threadLocalObjects = ThreadLocal.withInitial(LoadedObjectMap::new); + private LoadedObjectResolver() { + } + public static void begin() { LoadedObjectMap map = threadLocalObjects.get(); map.referenceCount++; @@ -23,7 +21,7 @@ public static void end() { LoadedObjectMap map = threadLocalObjects.get(); map.referenceCount--; if (map.referenceCount == 0) { - map.objectMap.clear(); + threadLocalObjects.remove(); } } @@ -39,4 +37,9 @@ public static Object get(Key key) { LoadedObjectMap map = threadLocalObjects.get(); return map.objectMap.get(key); } -} \ No newline at end of file + + private static class LoadedObjectMap { + private final Map objectMap = new HashMap<>(); + private int referenceCount = 0; + } +} diff --git a/src/main/java/com/aerospike/mapper/tools/ReactiveAeroMapper.java b/src/main/java/com/aerospike/mapper/tools/ReactiveAeroMapper.java index 4e30c6c..5094829 100644 --- a/src/main/java/com/aerospike/mapper/tools/ReactiveAeroMapper.java +++ b/src/main/java/com/aerospike/mapper/tools/ReactiveAeroMapper.java @@ -1,11 +1,5 @@ package com.aerospike.mapper.tools; -import java.util.Arrays; -import java.util.Objects; -import java.util.function.Function; - -import javax.validation.constraints.NotNull; - import com.aerospike.client.AerospikeException; import com.aerospike.client.Bin; import com.aerospike.client.Key; @@ -24,28 +18,20 @@ import com.aerospike.mapper.tools.converters.MappingConverter; import com.aerospike.mapper.tools.utils.MapperUtils; import com.aerospike.mapper.tools.virtuallist.ReactiveVirtualList; - import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import javax.validation.constraints.NotNull; +import java.util.Arrays; +import java.util.Objects; +import java.util.function.Function; + public class ReactiveAeroMapper implements IReactiveAeroMapper { private final IAerospikeReactorClient reactorClient; private final IAeroMapper aeroMapper; private final MappingConverter mappingConverter; - /** - * Create a new Builder to instantiate the AeroMapper. - * @author tfaulkes - * - */ - public static class Builder extends AbstractBuilder { - public Builder(IAerospikeReactorClient reactorClient) { - super(new ReactiveAeroMapper(reactorClient)); - ClassCache.getInstance().setReactiveDefaultPolicies(reactorClient); - } - } - private ReactiveAeroMapper(@NotNull IAerospikeReactorClient reactorClient) { this.reactorClient = reactorClient; this.aeroMapper = new AeroMapper.Builder(reactorClient.getAerospikeClient()).build(); @@ -205,7 +191,15 @@ public Flux read(BatchPolicy batchPolicy, @NotNull Class clazz, @NotNu return readBatch(batchPolicy, clazz, keys, entry, operations); } - private Mono read(Policy readPolicy, @NotNull Class clazz, @NotNull Key key, @NotNull ClassCacheEntry entry, boolean resolveDependencies) { + @SuppressWarnings("unchecked") + private Mono read(Policy readPolicy, @NotNull Class clazz, @NotNull Key key, + @NotNull ClassCacheEntry entry, boolean resolveDependencies) { + if (readPolicy == null || readPolicy.filterExp == null) { + Object objectForKey = LoadedObjectResolver.get(key); + if (objectForKey != null) { + return Mono.just((T) objectForKey); + } + } if (readPolicy == null) { readPolicy = entry.getReadPolicy(); } @@ -368,7 +362,8 @@ public ReactiveVirtualList asBackedList(@NotNull Object object, @NotNull } @Override - public ReactiveVirtualList asBackedList(@NotNull Class owningClazz, @NotNull Object key, @NotNull String binName, Class elementClazz) { + public ReactiveVirtualList asBackedList(@NotNull Class owningClazz, @NotNull Object key, + @NotNull String binName, Class elementClazz) { return new ReactiveVirtualList<>(this, owningClazz, key, binName, elementClazz); } @@ -454,4 +449,14 @@ public Mono getRecordKey(Object obj) { ClassCacheEntry entry = ClassCache.getInstance().loadClass(obj.getClass(), this); return entry == null ? null : Mono.just(new Key(entry.getNamespace(), entry.getSetName(), Value.get(entry.getKey(obj)))); } + + /** + * Create a new Builder to instantiate the AeroMapper. + */ + public static class Builder extends AbstractBuilder { + public Builder(IAerospikeReactorClient reactorClient) { + super(new ReactiveAeroMapper(reactorClient)); + ClassCache.getInstance().setReactiveDefaultPolicies(reactorClient); + } + } } diff --git a/src/main/java/com/aerospike/mapper/tools/ThreadLocalKeySaver.java b/src/main/java/com/aerospike/mapper/tools/ThreadLocalKeySaver.java index 0bf22ec..c2860bc 100644 --- a/src/main/java/com/aerospike/mapper/tools/ThreadLocalKeySaver.java +++ b/src/main/java/com/aerospike/mapper/tools/ThreadLocalKeySaver.java @@ -1,24 +1,32 @@ package com.aerospike.mapper.tools; +import com.aerospike.client.Key; + import java.util.ArrayDeque; import java.util.Deque; -import com.aerospike.client.Key; - /** - * Save the keys. Note that this is effectively a stack of keys, as A can load B which can load C, and C needs B's key, not A's. - * - * @author timfaulkes + * Save the keys. Note that this is effectively a stack of keys, as A can load B which can load C, and C needs B's key, + * not A's. */ public class ThreadLocalKeySaver { + private static final ThreadLocal> threadLocalKeys = ThreadLocal.withInitial(ArrayDeque::new); + private ThreadLocalKeySaver() { + } + public static void save(Key key) { threadLocalKeys.get().addLast(key); + LoadedObjectResolver.begin(); } public static void clear() { + LoadedObjectResolver.end(); threadLocalKeys.get().removeLast(); + if (threadLocalKeys.get().isEmpty()) { + threadLocalKeys.remove(); + } } public static Key get() { diff --git a/src/test/java/com/aerospike/mapper/reactive/ReactiveRecursiveObjectTest.java b/src/test/java/com/aerospike/mapper/reactive/ReactiveRecursiveObjectTest.java new file mode 100644 index 0000000..57cb863 --- /dev/null +++ b/src/test/java/com/aerospike/mapper/reactive/ReactiveRecursiveObjectTest.java @@ -0,0 +1,62 @@ +package com.aerospike.mapper.reactive; + +import com.aerospike.mapper.RecursiveObjectTest; +import com.aerospike.mapper.tools.ReactiveAeroMapper; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +public class ReactiveRecursiveObjectTest extends ReactiveAeroMapperBaseTest { + + @Test + public void runTest() { + RecursiveObjectTest.A a1 = new RecursiveObjectTest.A("a", 10, 1); + a1.a = a1; + + ReactiveAeroMapper reactiveMapper = new ReactiveAeroMapper.Builder(reactorClient).build(); + reactiveMapper.save(a1).block(); + + RecursiveObjectTest.A a2 = reactiveMapper.read(RecursiveObjectTest.A.class, a1.id).block(); + assertNotNull(a2); + assertEquals(a1.age, a2.age); + assertEquals(a1.name, a2.name); + assertEquals(a1.id, a2.id); + } + + @Test + public void runMultipleObjectTest() { + RecursiveObjectTest.A a1 = new RecursiveObjectTest.A("a", 10, 1); + a1.a = a1; + RecursiveObjectTest.B b = new RecursiveObjectTest.B(); + b.id = 10; + b.a = a1; + + ReactiveAeroMapper reactiveMapper = new ReactiveAeroMapper.Builder(reactorClient).build(); + reactiveMapper.save(a1).block(); + reactiveMapper.save(b).block(); + + RecursiveObjectTest.B b2 = reactiveMapper.read(RecursiveObjectTest.B.class, b.id).block(); + assertNotNull(b2); + assertEquals(b.id, b2.id); + assertEquals(b.a.age, b2.a.age); + assertEquals(b.a.name, b2.a.name); + assertEquals(b.a.id, b2.a.id); + } + + @Test + public void runTest2() { + RecursiveObjectTest.A a1 = new RecursiveObjectTest.A("a", 10, 1); + a1.a = new RecursiveObjectTest.A("a2", 11, 11); + + ReactiveAeroMapper reactiveMapper = new ReactiveAeroMapper.Builder(reactorClient).build(); + reactiveMapper.save(a1, a1.a).blockLast(); + + RecursiveObjectTest.A a2 = reactiveMapper.read(RecursiveObjectTest.A.class, a1.id).block(); + assertNotNull(a2); + assertEquals(a1.age, a2.age); + assertEquals(a1.name, a2.name); + assertEquals(a1.id, a2.id); + assertEquals(a1.a.id, a2.a.id); + } +}