diff --git a/src/main/java/net/openhft/chronicle/hash/impl/ChronicleHashResources.java b/src/main/java/net/openhft/chronicle/hash/impl/ChronicleHashResources.java index 85a677cc0..b7bf9ab07 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/ChronicleHashResources.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/ChronicleHashResources.java @@ -24,8 +24,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.Closeable; +import java.io.IOException; import java.lang.ref.WeakReference; -import java.util.*; +import java.util.ArrayList; +import java.util.List; /** * ChronicleHashResources is Runnable to be passed as "hunk" to {@link sun.misc.Cleaner}. @@ -33,51 +35,59 @@ import java.util.*; public abstract class ChronicleHashResources implements Runnable { private static final Logger LOG = LoggerFactory.getLogger(ChronicleHashResources.class); + private static final int OPEN = 0; + private static final int PARTIALLY_CLOSED = 1; + private static final int COMPLETELY_CLOSED = 2; + /** - * closed field is volatile and {@link #closed()} accessor is not synchronized, because it - * is called on each ChronicleMap access from all threads, synchronization would make closed() - * a bottleneck. + * This field is volatile and {@link #closed()} accessor is not synchronized, because it + * is called on each {@link VanillaChronicleHash} access from all threads, synchronization would + * make {@code closed()} a bottleneck. */ - private volatile boolean closed = false; + private volatile int state = OPEN; private List memoryResources = new ArrayList<>(); private List closeables = new ArrayList<>(1); - private ArrayList> contexts = new ArrayList<>(); + private List> contexts = new ArrayList<>(); /** * Identity String of the ChronicleHash, for which this ChronicleHashResources is created. - * ChronicleHash couldn't be directly referenced, because it's a hunk for {@link - * sun.misc.Cleaner}, and it would prevent the chronicleHash from ever becoming unreachable. + * ChronicleHash couldn't be directly referenced, because {@code ChronicleHashResources} is a + * hunk for {@link sun.misc.Cleaner}, and it would prevent the chronicleHash from ever becoming + * unreachable. */ private String chronicleHashIdentityString; - List memoryResources() { - return memoryResources; - } - List> contexts() { return contexts; } final synchronized long totalMemory() { - if (closed) + if (closed()) return 0L; long totalMemory = 0L; - for (MemoryResource memoryResource : memoryResources) { - totalMemory += memoryResource.size; + //noinspection ForLoopReplaceableByForEach -- allocation-free looping + for (int i = 0; i < memoryResources.size(); i++) { + totalMemory += memoryResources.get(i).size; } return totalMemory; } final boolean closed() { - return closed; + return state != OPEN; } private void checkOpen() { - if (closed) + if (closed()) throw new ChronicleHashClosedException(chronicleHashIdentityString); } + /** + * Need to set {@link #chronicleHashIdentityString} via this setter rather than once in the + * constructor, because {@code ChronicleHashResources} instance is created before it's + * {@link net.openhft.chronicle.hash.ChronicleHash}. + */ public final synchronized void setChronicleHashIdentityString( String chronicleHashIdentityString) { + checkOpen(); this.chronicleHashIdentityString = chronicleHashIdentityString; } @@ -86,7 +96,7 @@ public abstract class ChronicleHashResources implements Runnable { memoryResources.add(new MemoryResource(address, size)); } - public final synchronized void addCloseable(Closeable closeable) { + final synchronized void addCloseable(Closeable closeable) { checkOpen(); closeables.add(closeable); } @@ -111,20 +121,20 @@ public abstract class ChronicleHashResources implements Runnable { public void run() { Throwable thrown = null; try { - if (closed) + if (state == COMPLETELY_CLOSED) return; try { - LOG.error("{} not closed manually, cleaned up from Cleaner", + LOG.error("{} is not closed manually, cleaned up from Cleaner", chronicleHashIdentityString); } catch (Throwable t) { thrown = t; } finally { synchronized (this) { - if (closed) { + if (state == COMPLETELY_CLOSED) { LOG.error("Somebody closed {} while it is processed by Cleaner, " + "this should be impossible", chronicleHashIdentityString); } else { - thrown = Throwables.returnOrSuppress(thrown, releaseEverything()); + thrown = Throwables.returnOrSuppress(thrown, releaseEverything(true)); } } if (thrown != null) { @@ -140,82 +150,152 @@ public abstract class ChronicleHashResources implements Runnable { } } } catch (Throwable ignore) { - // just don't fail anyway + // Just don't fail anyway. We will have another attempt to close this ChronicleMap from + // ChronicleHashCloseOnExitHook. } } - abstract Throwable releaseSystemResources(); + abstract void releaseMemoryResource(MemoryResource memoryResource) throws IOException; + + Throwable releaseExtraSystemResources() { + // nothing to release + return null; + } public final boolean releaseManually() { - if (closed) + if (state == COMPLETELY_CLOSED) return false; synchronized (this) { - if (closed) + if (state == COMPLETELY_CLOSED) return false; - Throwable thrown = releaseEverything(); + Throwable thrown = releaseEverything(false); if (thrown != null) throw Throwables.propagate(thrown); return true; } } - private Throwable releaseEverything() { - // It's important to set closed = true before calling closeContexts(), to ensure that - // threads which access this chronicleHash for the first time concurrently with + private Throwable releaseEverything(boolean releaseFromCleaner) { + // It's important to set state = PARTIALLY_CLOSED before calling closeContexts(), to ensure + // that threads which access this chronicleHash for the first time concurrently with // chronicleHash.close() will either fail to register the context via addContext() (because // checkOpen() is called there), or either the registered contexts will be visible and - // closed in closeContexts() - closed = true; + // closed in closeContexts(). Also, it allows to not care about null elements of + // memoryResources, contexts and closeables lists in addMemoryResource(), addContext() and + // addCloseable() respectively. + state = PARTIALLY_CLOSED; - // Paranoiac mode: methods closeContexts(), closeCloseables(), releaseSystemResources() and - // Throwables.returnOrSuppress() should never throw any throwables (the first three should - // return them instead of throwing), but we don't trust ourselves and wrap every call into - // it's own try-finally block + // Paranoiac mode: methods closeContexts(), closeCloseables(), releaseMemoryResources(), + // releaseExtraSystemResources() and Throwables.returnOrSuppress() should never throw any + // throwables (the first three should return them instead of throwing), but we don't trust + // ourselves and wrap every call into it's own try-finally block. + Throwable thrown = null; try { - Throwable thrown = null; - try { + if (contexts != null) thrown = closeContexts(); - } finally { - try { + } catch (Throwable t) { + thrown = t; + } finally { + try { + if (closeables != null) thrown = Throwables.returnOrSuppress(thrown, closeCloseables()); - } finally { - // It's important to close the system resources after contexts and closeables - // because the latter may use those system resources - thrown = Throwables.returnOrSuppress(thrown, releaseSystemResources()); + } catch (Throwable t) { + try { + thrown = Throwables.returnOrSuppress(thrown, t); + } catch (Throwable stackOverflowError) { + // It seems that only StackOverflowError could be thrown from + // Throwables.returnOrSuppress(), but in this case `thrown` and `t` are likely + // StackOverflowErrors too, so just return one of them without trying to call + // any more methods, including Throwable.addSuppressed(). + thrown = thrown != null ? thrown : t; } } - return thrown; - } finally { - // Make GC life easier - closeables = null; - contexts = null; - memoryResources = null; } + // It's important to close the system resources only after contexts and closeables are + // closed successfully, because the latter may use those system resources. However if + // releaseEverything() is called from Cleaner, the ChronicleMap object is already + // unreachable, that means no contexts are accessing it anymore and the problem with closing + // contexts could probably be StackOverflowError or other exotic Error, so in this case we + // try to close the system resources anyway. + if (thrown == null || releaseFromCleaner) { + try { + thrown = releaseSystemResources(thrown); + } catch (Throwable stackOverflowError) { + // It seems that only StackOverflowError could be thrown (rather than returned!) + // from releaseSystemResources(), so just return any Throwable from + // releaseEverything() without trying to call any more methods. + thrown = thrown != null ? thrown : stackOverflowError; + } + } + if (thrown == null) { + state = COMPLETELY_CLOSED; + } + return thrown; + } + + private Throwable releaseSystemResources(Throwable thrown) { + try { + if (memoryResources != null) + thrown = Throwables.returnOrSuppress(thrown, releaseMemoryResources()); + } catch (Throwable t) { + thrown = Throwables.returnOrSuppress(thrown, t); + } finally { + try { + thrown = Throwables.returnOrSuppress(thrown, releaseExtraSystemResources()); + } catch (Throwable t) { + try { + thrown = Throwables.returnOrSuppress(thrown, t); + } catch (Throwable stackOverflowError) { + // It seems that only StackOverflowError could be thrown from + // Throwables.returnOrSuppress(), but in this case `thrown` and `t` are likely + // StackOverflowErrors too, so just return one of them without trying to call + // any more methods, including Throwable.addSuppressed(). + thrown = thrown != null ? thrown : t; + } + } + } + return thrown; } private Throwable closeContexts() { Throwable thrown = null; - // Indexed loop instead of for-each because we may be in the context of cleaning up after - // OutOfMemoryError, and allocating Iterator object may lead to another OutOfMemoryError, so - // we try to avoid any allocations - //noinspection ForLoopReplaceableByForEach + List> contexts = this.contexts; + // Indexed loop instead of using Iterator because we may be in the context of cleaning up + // after OutOfMemoryError, and allocating Iterator object may lead to another + // OutOfMemoryError, so we try to avoid any allocations. for (int i = 0; i < contexts.size(); i++) { WeakReference contextHolderRef = contexts.get(i); - try { - ContextHolder contextHolder = contextHolderRef.get(); - if (contextHolder != null) { - closeContext(contextHolder); + // The context reference could have already been set to null in (*). See comments in + // closeContext() with more explanations. + if (contextHolderRef != null) { + try { + ContextHolder contextHolder = contextHolderRef.get(); + if (contextHolder != null) { + closeContext(contextHolder); + } + // (*) Make GC life easier + contexts.set(i, null); + } catch (Throwable t) { + thrown = Throwables.returnOrSuppress(thrown, t); } - } catch (Throwable t) { - thrown = Throwables.returnOrSuppress(thrown, t); } } + // Forget about contexts only if all of them are successfully closed, i. e. no throwables + // were thrown in the above loop. + if (thrown == null) { + // Make GC life easier + this.contexts = null; + } return thrown; } private void closeContext(ContextHolder contextHolder) { ChainingInterface context = contextHolder.get(); - try { + // The context could have already been cleared, if this is the second attempt to close + // contexts, the first one failed e. g. with IllegalStateException on one of the contexts + // (see comment (*) below in this method), it could have succeed for some contexts and + // contextHolder.clear() is performed. + if (context != null) { if (context.owner().isAlive()) { // Ensures that if the thread owning this context will come to access chronicleHash // concurrently with resource releasing operation, it will fail due to the check in @@ -224,26 +304,74 @@ public abstract class ChronicleHashResources implements Runnable { // this access session. context.closeContext(chronicleHashIdentityString); } - } finally { - // See ContextHolder's class-level Javadoc comment which explains motivation for this + + // (*) Don't execute contextHolder.clear() from a finally section of a try block + // wrapping context.closeContext(), because if context.closeContext() fails e. g. with + // IllegalStateException because the context is currently used (this happens if + // ChronicleMap.close() is called within try-with-resources block of it's own operation, + // see MapCloseTest.closeInContextTest()), we may want to try to close this context + // again from ChronicleHashCloseOnExitHook. + + // See ContextHolder's class-level Javadoc comment that explains the motivation for + // calling contextHolder.clear(). contextHolder.clear(); } } private Throwable closeCloseables() { Throwable thrown = null; - // Indexed loop instead of for-each because we may be in the context of cleaning up after - // OutOfMemoryError, and allocating Iterator object may lead to another OutOfMemoryError, so - // we try to avoid any allocations - //noinspection ForLoopReplaceableByForEach + List closeables = this.closeables; + // Indexed loop instead of using Iterator because we may be in the context of cleaning up + // after OutOfMemoryError, and allocating Iterator object may lead to another + // OutOfMemoryError, so we try to avoid any allocations. for (int i = 0; i < closeables.size(); i++) { Closeable closeable = closeables.get(i); - try { - closeable.close(); - } catch (Throwable t) { - thrown = Throwables.returnOrSuppress(thrown, t); + // The closeable could have already been set to null in (*), see comments in + // closeContexts() method which has similar logic. + if (closeable != null) { + try { + closeable.close(); + // (*) Make GC life easier + closeables.set(i, null); + } catch (Throwable t) { + thrown = Throwables.returnOrSuppress(thrown, t); + } } } + // Forget about closeables only if all of them are successfully closed, i. e. no throwables + // were thrown in the above loop. + if (thrown == null) { + // Make GC life easier + this.closeables = null; + } + return thrown; + } + + private Throwable releaseMemoryResources() { + Throwable thrown = null; + List memoryResources = this.memoryResources; + // Indexed loop instead of using Iterator because we may be in the context of cleaning up + // after OutOfMemoryError, and allocating Iterator object may lead to another + // OutOfMemoryError, so we try to avoid any allocations. + for (int i = 0; i < memoryResources.size(); i++) { + MemoryResource memoryResource = memoryResources.get(i); + // The memory resource could have already been nulled out in (*), see comments in + // closeCloseables() method which has similar logic. + if (memoryResource != null) { + try { + releaseMemoryResource(memoryResource); + // (*) Make GC life easier + memoryResources.set(i, null); + } catch (Throwable t) { + thrown = Throwables.returnOrSuppress(thrown, t); + } + } + } + // Forget about memory resources only if all of them are successfully released, i. e. no + // throwables were thrown in the above loop. + if (thrown == null) { + this.memoryResources = null; + } return thrown; } } diff --git a/src/main/java/net/openhft/chronicle/hash/impl/InMemoryChronicleHashResources.java b/src/main/java/net/openhft/chronicle/hash/impl/InMemoryChronicleHashResources.java index 9f60feae0..c3fc5701d 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/InMemoryChronicleHashResources.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/InMemoryChronicleHashResources.java @@ -23,25 +23,8 @@ import net.openhft.chronicle.hash.impl.util.Throwables; import java.util.List; public final class InMemoryChronicleHashResources extends ChronicleHashResources { - - public InMemoryChronicleHashResources() {} - @Override - Throwable releaseSystemResources() { - Throwable thrown = null; - List memoryResources = memoryResources(); - // Indexed loop instead of for-each because we may be in the context of cleaning up after - // OutOfMemoryError, and allocating Iterator object may lead to another OutOfMemoryError, so - // we try to avoid any allocations - //noinspection ForLoopReplaceableByForEach - for (int i = 0; i < memoryResources.size(); i++) { - MemoryResource allocation = memoryResources.get(i); - try { - OS.memory().freeMemory(allocation.address, allocation.size); - } catch (Throwable t) { - thrown = Throwables.returnOrSuppress(thrown, t); - } - } - return thrown; + void releaseMemoryResource(MemoryResource allocation) { + OS.memory().freeMemory(allocation.address, allocation.size); } } diff --git a/src/main/java/net/openhft/chronicle/hash/impl/PersistedChronicleHashResources.java b/src/main/java/net/openhft/chronicle/hash/impl/PersistedChronicleHashResources.java index 7d14f084d..e9ca85e61 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/PersistedChronicleHashResources.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/PersistedChronicleHashResources.java @@ -19,56 +19,35 @@ package net.openhft.chronicle.hash.impl; import net.openhft.chronicle.core.OS; import net.openhft.chronicle.hash.impl.util.CanonicalRandomAccessFiles; -import net.openhft.chronicle.hash.impl.util.Throwables; import java.io.File; -import java.util.List; +import java.io.IOException; public final class PersistedChronicleHashResources extends ChronicleHashResources { - private final File file; + private File file; public PersistedChronicleHashResources(File file) { this.file = file; + OS.memory().storeFence(); // Emulate final semantics of the file field } @Override - Throwable releaseSystemResources() { - Throwable thrown = null; - // Paranoiac mode: releaseMappedMemory() should never throw any Throwable (it should return - // it), but we don't trust ourselves and call releaseFile() in finally block. - try { - thrown = releaseMappedMemory(); - } finally { - thrown = releaseFile(thrown); - } - return thrown; + void releaseMemoryResource(MemoryResource mapping) throws IOException { + OS.unmap(mapping.address, mapping.size); } - private Throwable releaseMappedMemory() { + @Override + Throwable releaseExtraSystemResources() { + if (file == null) + return null; Throwable thrown = null; - List memoryResources = memoryResources(); - // Indexed loop instead of for-each because we may be in the context of cleaning up after - // OutOfMemoryError, and allocating Iterator object may lead to another OutOfMemoryError, so - // we try to avoid any allocations - //noinspection ForLoopReplaceableByForEach - for (int i = 0; i < memoryResources.size(); i++) { - MemoryResource mapping = memoryResources.get(i); - try { - OS.unmap(mapping.address, mapping.size); - } catch (Throwable t) { - thrown = Throwables.returnOrSuppress(thrown, t); - } - } - return thrown; - } - - private Throwable releaseFile(Throwable thrown) { try { CanonicalRandomAccessFiles.release(file); + file = null; } catch (Throwable t) { - thrown = Throwables.returnOrSuppress(thrown, t); + thrown = t; } return thrown; } diff --git a/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java b/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java index 5286fc2ea..90f714158 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java @@ -657,6 +657,8 @@ public abstract class VanillaChronicleHash { - if (ref == null) - throw new IllegalStateException("releasing not referenced RAF of file " + f); + canonicalRafs.computeIfPresent(file, (f, ref) -> { if (--ref.refCount == 0) { try { ref.raf.close(); diff --git a/src/main/java/net/openhft/chronicle/hash/impl/util/Throwables.java b/src/main/java/net/openhft/chronicle/hash/impl/util/Throwables.java index 4bc75187a..c2c121209 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/util/Throwables.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/util/Throwables.java @@ -23,7 +23,9 @@ package net.openhft.chronicle.hash.impl.util; public final class Throwables { public static RuntimeException propagate(Throwable t) { - Objects.requireNonNull(t); + // Avoid calling Objects.requireNonNull(), StackOverflowError-sensitive + if (t == null) + throw new NullPointerException(); if (t instanceof Error) throw (Error) t; if (t instanceof RuntimeException) diff --git a/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java b/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java index 18628118c..17c13877c 100755 --- a/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java +++ b/src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java @@ -335,9 +335,9 @@ public class VanillaChronicleMap cxt.set(contextHolder); return iterationContext; } - ChainingInterface iterContext = contextHolder.get(); - if (iterContext != null) { - return iterContext; + ChainingInterface iterationContext = contextHolder.get(); + if (iterationContext != null) { + return iterationContext; } else { throw new ChronicleHashClosedException(this); } diff --git a/src/test/java/net/openhft/chronicle/map/ExitHookTest.java b/src/test/java/net/openhft/chronicle/map/ExitHookTest.java index 000ea47f9..1aad7647e 100644 --- a/src/test/java/net/openhft/chronicle/map/ExitHookTest.java +++ b/src/test/java/net/openhft/chronicle/map/ExitHookTest.java @@ -61,7 +61,11 @@ public class ExitHookTest { ChronicleMap map = createMap(mapFile); try (ExternalMapQueryContext c = map.queryContext(KEY)) { c.writeLock().lock(); - Thread.sleep(30_000); + try { + map.close(); + } finally { + Thread.sleep(30_000); + } } } diff --git a/src/test/java/net/openhft/chronicle/map/MemoryLeaksTest.java b/src/test/java/net/openhft/chronicle/map/MemoryLeaksTest.java index 2ad39bc99..3e5b98728 100644 --- a/src/test/java/net/openhft/chronicle/map/MemoryLeaksTest.java +++ b/src/test/java/net/openhft/chronicle/map/MemoryLeaksTest.java @@ -17,11 +17,13 @@ package net.openhft.chronicle.map; +import com.google.common.collect.Lists; import net.openhft.chronicle.bytes.NoBytesStore; import net.openhft.chronicle.core.OS; import net.openhft.chronicle.core.values.IntValue; import net.openhft.chronicle.hash.serialization.impl.StringSizedReader; import net.openhft.chronicle.hash.serialization.impl.StringUtf8DataAccess; +import net.openhft.chronicle.map.impl.QueryContextInterface; import net.openhft.chronicle.values.Values; import org.junit.Assert; import org.junit.Before; @@ -33,10 +35,15 @@ import org.junit.runners.Parameterized; import sun.misc.Cleaner; import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; import java.lang.ref.WeakReference; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.List; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; @@ -45,27 +52,29 @@ public class MemoryLeaksTest { @Parameterized.Parameters public static Collection data() { - return Arrays.asList(new Object[][] { - { false, false }, { false, true }, { true, false }, { true, true } - }); + List booleans = Arrays.asList(false, true); + // Test with all possible combinations of three boolean parameters. + return Lists.cartesianProduct(booleans, booleans, booleans) + .stream().map(List::toArray).collect(Collectors.toList()); } @Rule public TemporaryFolder folder = new TemporaryFolder(); - private boolean replicated; private boolean persisted; private ChronicleMapBuilder builder; + private boolean closeWithinContext; /** * Accounting {@link CountedStringReader} creation and finalization. All serializers, * created since the map creation, should become unreachable after map.close() or collection by * Cleaner, it means that map contexts (referencing serializers) are collected by the GC */ private final AtomicInteger serializerCount = new AtomicInteger(); + private final List> serializers = new ArrayList<>(); - public MemoryLeaksTest(boolean replicated, boolean persisted) { - this.replicated = replicated; + public MemoryLeaksTest(boolean replicated, boolean persisted, boolean closeWithinContext) { this.persisted = persisted; + this.closeWithinContext = closeWithinContext; builder = ChronicleMap .of(IntValue.class, String.class) .valueReaderAndDataAccess(new CountedStringReader(), new StringUtf8DataAccess()); @@ -89,10 +98,13 @@ public class MemoryLeaksTest { throws IOException, InterruptedException { long nativeMemoryUsedBeforeMap = nativeMemoryUsed(); int serializersBeforeMap = serializerCount.get(); - WeakReference> ref = new WeakReference<>(getMap()); - Assert.assertNotNull(ref.get()); - long expectedNativeMemory = nativeMemoryUsedBeforeMap + ref.get().offHeapMemoryUsed(); + ChronicleMap map = getMap(); + long expectedNativeMemory = nativeMemoryUsedBeforeMap + map.offHeapMemoryUsed(); assertEquals(expectedNativeMemory, nativeMemoryUsed()); + tryCloseFromContext(map); + WeakReference> ref = new WeakReference<>(map); + Assert.assertNotNull(ref.get()); + map = null; // Wait until Map is collected by GC while (ref.get() != null) { System.gc(); @@ -120,17 +132,40 @@ public class MemoryLeaksTest { } } - @Test + @Test(timeout = 60_000) public void testExplicitChronicleMapCloseReleasesMemory() throws IOException, InterruptedException { long nativeMemoryUsedBeforeMap = nativeMemoryUsed(); int serializersBeforeMap = serializerCount.get(); - try (ChronicleMap map = getMap()) { + ChronicleMap map = getMap(); + // One serializer should be copied to the map's valueReader field, another is copied from + // the map's valueReader field to the context + Assert.assertTrue(serializerCount.get() >= serializersBeforeMap + 2); + Assert.assertNotEquals(0, map.offHeapMemoryUsed()); + try { long expectedNativeMemory = nativeMemoryUsedBeforeMap + map.offHeapMemoryUsed(); assertEquals(expectedNativeMemory, nativeMemoryUsed()); + } finally { + tryCloseFromContext(map); + map.close(); } assertEquals(nativeMemoryUsedBeforeMap, nativeMemoryUsed()); // Wait until chronicle map context (hence serializers) is collected by the GC + for (int i = 0; i < 6_000; i++) { + if (serializerCount.get() == serializersBeforeMap + 1) + break; + System.gc(); + byte[] garbage = new byte[10_000_000]; + Thread.sleep(10); + } + Assert.assertEquals(serializersBeforeMap + 1, serializerCount.get()); + // This assertion ensures GC doesn't reclaim the map before or during the loop iteration + // above, to ensure that we test that the direct memory and contexts are released because + // of the manual map.close(), despite the "leak" of the map object itself. + Assert.assertEquals(0, map.offHeapMemoryUsed()); + map = null; + // After cleaning the reference to the map, the last serializer, which was held in it's + // field, should be cleaned as well for (int i = 0; i < 6_000; i++) { if (serializerCount.get() == serializersBeforeMap) break; @@ -158,13 +193,39 @@ public class MemoryLeaksTest { return map; } + private void tryCloseFromContext(ChronicleMap map) { + // Test that the map could still be successfully closed and no leaks are introduced + // by an attempt to close the map from within context. + if (closeWithinContext) { + IntValue key = Values.newHeapInstance(IntValue.class); + try (ExternalMapQueryContext c = map.queryContext(key)) { + c.updateLock().lock(); + try { + map.close(); + } catch (IllegalStateException expected) { + // expected + } + } + } + } + private class CountedStringReader extends StringSizedReader { + private final String creationStackTrace; private final Cleaner cleaner; CountedStringReader() { serializerCount.incrementAndGet(); + serializers.add(new WeakReference<>(this)); cleaner = Cleaner.create(this, serializerCount::decrementAndGet); + try (StringWriter stringWriter = new StringWriter(); + PrintWriter printWriter = new PrintWriter(stringWriter)) { + new Exception().printStackTrace(printWriter); + printWriter.flush(); + creationStackTrace = stringWriter.toString(); + } catch (IOException e) { + throw new RuntimeException(e); + } } @Override