Merge pull request #1139 from katzyn/closer

Fix a memory leak caused by DatabaseCloser objects

h2/src/main/org/h2/engine/Database.java

@@ -157,7 +157,7 @@ public class Database implements DataHandler {
 
     private int powerOffCount = initialPowerOffCount;
     private int closeDelay;
-    private DatabaseCloser delayedCloser;
+    private DelayedDatabaseCloser delayedCloser;
     private volatile boolean closing;
     private boolean ignoreCase;
     private boolean deleteFilesOnDisconnect;
@@ -168,7 +168,6 @@ public class Database implements DataHandler {
     private boolean referentialIntegrity = true;
     /** ie. the MVCC setting */
     private boolean multiVersion;
-    private DatabaseCloser closeOnExit;
     private Mode mode = Mode.getRegular();
     private boolean multiThreaded;
     private int maxOperationMemory =
@@ -289,18 +288,7 @@ public class Database implements DataHandler {
         try {
             open(traceLevelFile, traceLevelSystemOut, ci);
             if (closeAtVmShutdown) {
-                try {
-                    closeOnExit = new DatabaseCloser(this, 0, true);
-                    Runtime.getRuntime().addShutdownHook(closeOnExit);
-                } catch (IllegalStateException e) {
-                    // shutdown in progress - just don't register the handler
-                    // (maybe an application wants to write something into a
-                    // database at shutdown time)
-                } catch (SecurityException  e) {
-                    // applets may not do that - ignore
-                    // Google App Engine doesn't allow
-                    // to instantiate classes that extend Thread
-                }
+                OnExitDatabaseCloser.register(this);
             }
         } catch (Throwable e) {
             if (e instanceof OutOfMemoryError) {
@@ -1240,10 +1228,7 @@ public class Database implements DataHandler {
             } else if (closeDelay < 0) {
                 return;
             } else {
-                delayedCloser = new DatabaseCloser(this, closeDelay * 1000, false);
-                delayedCloser.setName("H2 Close Delay " + getShortName());
-                delayedCloser.setDaemon(true);
-                delayedCloser.start();
+                delayedCloser = new DelayedDatabaseCloser(this, closeDelay * 1000);
             }
         }
         if (session != systemSession &&
@@ -1361,17 +1346,7 @@ public class Database implements DataHandler {
             }
             trace.info("closed");
             traceSystem.close();
-            if (closeOnExit != null) {
-                closeOnExit.reset();
-                try {
-                    Runtime.getRuntime().removeShutdownHook(closeOnExit);
-                } catch (IllegalStateException e) {
-                    // ignore
-                } catch (SecurityException e) {
-                    // applets may not do that - ignore
-                }
-                closeOnExit = null;
-            }
+            OnExitDatabaseCloser.unregister(this);
             if (deleteFilesOnDisconnect && persistent) {
                 deleteFilesOnDisconnect = false;
                 try {

h2/src/main/org/h2/engine/DatabaseCloser.java → h2/src/main/org/h2/engine/DelayedDatabaseCloser.java

@@ -10,27 +10,28 @@ import java.lang.ref.WeakReference;
 import org.h2.message.Trace;
 
 /**
- * This class is responsible to close a database if the application did not
- * close a connection. A database closer object only exists if there is no user
- * connected to the database.
+ * This class is responsible to close a database after the specified delay. A
+ * database closer object only exists if there is no user connected to the
+ * database.
  */
-class DatabaseCloser extends Thread {
+class DelayedDatabaseCloser extends Thread {
 
-    private final boolean shutdownHook;
     private final Trace trace;
     private volatile WeakReference<Database> databaseRef;
     private int delayInMillis;
 
-    DatabaseCloser(Database db, int delayInMillis, boolean shutdownHook) {
-        this.databaseRef = new WeakReference<>(db);
+    DelayedDatabaseCloser(Database db, int delayInMillis) {
+        databaseRef = new WeakReference<>(db);
         this.delayInMillis = delayInMillis;
-        this.shutdownHook = shutdownHook;
         trace = db.getTrace(Trace.DATABASE);
+        setName("H2 Close Delay " + db.getShortName());
+        setDaemon(true);
+        start();
     }
 
     /**
-     * Stop and disable the database closer. This method is called after the
-     * database has been closed, or after a session has been created.
+     * Stop and disable the database closer. This method is called after a session
+     * has been created.
      */
     void reset() {
         databaseRef = null;
@@ -46,18 +47,16 @@ class DatabaseCloser extends Thread {
             } catch (Exception e) {
                 // ignore InterruptedException
             }
-            if (databaseRef == null) {
+            WeakReference<Database> ref = databaseRef;
+            if (ref == null || ref.get() == null) {
                 return;
             }
         }
-        Database database = null;
-        WeakReference<Database> ref = this.databaseRef;
-        if (ref != null) {
-            database = ref.get();
-        }
-        if (database != null) {
+        Database database;
+        WeakReference<Database> ref = databaseRef;
+        if (ref != null && (database = ref.get()) != null) {
             try {
-                database.close(shutdownHook);
+                database.close(false);
             } catch (RuntimeException e) {
                 // this can happen when stopping a web application,
                 // if loading classes is no longer allowed

h2/src/main/org/h2/engine/OnExitDatabaseCloser.java

+/*
+ * Copyright 2004-2018 H2 Group. Multiple-Licensed under the MPL 2.0,
+ * and the EPL 1.0 (http://h2database.com/html/license.html).
+ * Initial Developer: H2 Group
+ */
+package org.h2.engine;
+
+import java.util.WeakHashMap;
+
+import org.h2.message.Trace;
+
+/**
+ * This class is responsible to close a database on JVM shutdown.
+ */
+class OnExitDatabaseCloser extends Thread {
+
+    private static final WeakHashMap<Database, Void> DATABASES = new WeakHashMap<>();
+
+    private static OnExitDatabaseCloser INSTANCE;
+
+    static synchronized void register(Database db) {
+        DATABASES.put(db, null);
+        if (INSTANCE == null) {
+            try {
+                // Assign INSTANCE unconditionally to avoid further attempts to register a
+                // shutdown hook in case of exception.
+                Runtime.getRuntime().addShutdownHook(INSTANCE = new OnExitDatabaseCloser());
+            } catch (IllegalStateException e) {
+                // shutdown in progress - just don't register the handler
+                // (maybe an application wants to write something into a
+                // database at shutdown time)
+            } catch (SecurityException e) {
+                // applets may not do that - ignore
+                // Google App Engine doesn't allow
+                // to instantiate classes that extend Thread
+            }
+        }
+    }
+
+    static synchronized void unregister(Database db) {
+        DATABASES.remove(db);
+        if (DATABASES.isEmpty() && INSTANCE != null) {
+            try {
+                Runtime.getRuntime().removeShutdownHook(INSTANCE);
+            } catch (IllegalStateException e) {
+                // ignore
+            } catch (SecurityException e) {
+                // applets may not do that - ignore
+            }
+            INSTANCE = null;
+        }
+    }
+
+    private OnExitDatabaseCloser() {
+    }
+
+    @Override
+    public void run() {
+        RuntimeException root = null;
+        for (Database database : DATABASES.keySet()) {
+            try {
+                database.close(true);
+            } catch (RuntimeException e) {
+                // this can happen when stopping a web application,
+                // if loading classes is no longer allowed
+                // it would throw an IllegalStateException
+                try {
+                    database.getTrace(Trace.DATABASE).error(e, "could not close the database");
+                    // if this was successful, we ignore the exception
+                    // otherwise not
+                } catch (Throwable e2) {
+                    e.addSuppressed(e2);
+                    if (root == null) {
+                        root = e;
+                    } else {
+                        root.addSuppressed(e);
+                    }
+                }
+            }
+        }
+        if (root != null) {
+            throw root;
+        }
+    }
+
+}