Improvements to ChronicleMap resource closing subsystem, making it more robust in exotic scenarios (OutOfMemoryError, StackOverflowError) and be able to close after incorrect attempt to close when a context is open in the same thread

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<MemoryResource> memoryResources = new ArrayList<>();
     private List<Closeable> closeables = new ArrayList<>(1);
-    private ArrayList<WeakReference<ContextHolder>> contexts = new ArrayList<>();
+    private List<WeakReference<ContextHolder>> 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<MemoryResource> memoryResources() {
-        return memoryResources;
-    }
-
     List<WeakReference<ContextHolder>> 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<WeakReference<ContextHolder>> 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<ContextHolder> 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<Closeable> 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<MemoryResource> 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;
     }
 }

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<MemoryResource> 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);
     }
 }

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<MemoryResource> 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;
     }

src/main/java/net/openhft/chronicle/hash/impl/VanillaChronicleHash.java

@@ -657,6 +657,8 @@ public abstract class VanillaChronicleHash<K,
     @Override
     public final void close() {
         if (resources.releaseManually()) {
+            // Don't try to execute cleaner.clean() and ChronicleHashCloseOnExitHook.remove() from
+            // finally section of try block wrapping resources.releaseManually(), because
             // Releases nothing after resources.releaseManually(), only removes the cleaner
             // from the internal linked list of all cleaners.
             cleaner.clean();

src/main/java/net/openhft/chronicle/hash/impl/stage/hash/ThreadLocalState.java

@@ -23,7 +23,6 @@ import net.openhft.chronicle.hash.ChronicleHash;
 import net.openhft.chronicle.hash.ChronicleHashClosedException;
 
 import java.lang.reflect.Field;
-import java.util.Arrays;
 import java.util.concurrent.TimeUnit;
 
 import static net.openhft.chronicle.hash.impl.BigSegmentHeader.LOCK_TIMEOUT_SECONDS;
@@ -65,9 +64,9 @@ public abstract class ThreadLocalState {
         } else {
             if (contextLock == CONTEXT_LOCKED_LOCALLY)
                 return false;
-            // Don't extract this hash().isOpen() and the one above, because they could different
-            // results (the first (above) could return true, the second (below) - false).
-            if (!hash.isOpen())
+            // Don't extract this hash().isOpen() and the one above, because they could return
+            // different results: the first (above) could return true, the second (below) - false.
+            if (contextLock == CONTEXT_CLOSED || !hash.isOpen())
                 throw new ChronicleHashClosedException(hash);
             throw new AssertionError("Unknown context lock state: " + contextLock);
         }
@@ -84,6 +83,15 @@ public abstract class ThreadLocalState {
     public void closeContext(String chronicleHashIdentityString) {
         if (tryCloseContext())
             return;
+        // Unless there are bugs in this codebase, it could happen that
+        // contextLock == CONTEXT_CLOSED here only if closeContext() has succeed, and the subsequent
+        // contextHolder.clear() has failed in ChronicleHashResources.closeContext(), though this is
+        // hardly imaginable: contextHolder.clear() couldn't fail with OutOfMemoryError (because
+        // there are no allocations in this method) and StackOverflowError (because in this case
+        // closeContext() would fail with StackOverflowError before). But anyway it's probably
+        // a good idea to make this check rather than not to make.
+        if (contextLock == CONTEXT_CLOSED)
+            return;
         // If first attempt of closing a context (i. e. moving from unused to closed state) failed,
         // it means that the context is still in use. If this context belongs to the current thread,
         // this is a bug, because we cannot "wait" until context is unused in the same thread:
@@ -102,6 +110,10 @@ public abstract class ThreadLocalState {
         do {
             if (tryCloseContext())
                 return;
+            // Unless there are bugs in this codebase, this should never happen. But anyway it's
+            // probably a good idea to make this check rather than not to make.
+            if (contextLock == CONTEXT_CLOSED)
+                return;
             Thread.yield();
             long now = System.currentTimeMillis();
             if (now != lastTime) {

src/main/java/net/openhft/chronicle/hash/impl/util/CanonicalRandomAccessFiles.java

@@ -56,9 +56,7 @@ public final class CanonicalRandomAccessFiles {
     }
 
     public static void release(File file) throws IOException {
-        canonicalRafs.compute(file, (f, ref) -> {
-            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();

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)

src/main/java/net/openhft/chronicle/map/VanillaChronicleMap.java

@@ -335,9 +335,9 @@ public class VanillaChronicleMap<K, V, R>
             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);
         }