[#993] Register an entry container's configuration listeners only once it has opened

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java

@@ -1104,8 +1104,12 @@ public void close()
   {
     if (db != null)
     {
-      DirectoryServer.deregisterMonitorProvider(monitor);
-      monitor = null;
+      // Not yet registered when a failed open got no further than the database itself.
+      if (monitor != null)
+      {
+        DirectoryServer.deregisterMonitorProvider(monitor);
+        monitor = null;
+      }
       try
       {
         db.close();
@@ -1126,6 +1130,10 @@ public void close()
       {
         memQuota.releaseMemory(memQuota.memPercentToBytes(config.getDBCachePercent()));
       }
+      // Released once: what an open takes, the next open takes again, and a close which follows
+      // a close - BackendImpl.importLDIF closes the storage of its root container however the
+      // import ended, on top of the close the import itself made - releases nothing more.
+      memQuota = null;
     }
     config.removePDBChangeListener(this);
     if (diskMonitor != null)
@@ -1148,7 +1156,52 @@ public void open(AccessMode accessMode) throws ConfigException, StorageRuntimeEx
       // Do not open volume on disk
       return;
     }
-    open0(buildConfiguration(accessMode));
+    rejectIfOpen();
+    openOrGiveBack(buildConfiguration(accessMode));
+  }
+
+  /**
+   * Refuses to open a database which is open, before anything is taken for the attempt: the
+   * refusal guards against a programming error, and what this storage holds is left as it is.
+   */
+  private void rejectIfOpen()
+  {
+    if (db != null)
+    {
+      throw new IllegalStateException(
+          "Database is already open, either the backend is enabled or an import is currently running.");
+    }
+  }
+
+  /**
+   * Opens the database, or gives back what the attempt took before it failed. Nothing else will: a
+   * root container does not close a storage whose {@code open()} threw, and a backend whose open
+   * failed is thrown away with the storage still registered as a listener of its configuration and
+   * the cache size it reserved still drawn from the memory quota - once per attempt to enable it.
+   */
+  private void openOrGiveBack(final Configuration dbCfg) throws ConfigException
+  {
+    boolean opened = false;
+    try
+    {
+      open0(dbCfg);
+      opened = true;
+    }
+    finally
+    {
+      if (!opened)
+      {
+        try
+        {
+          close();
+        }
+        catch (RuntimeException e)
+        {
+          // The failure being given up after is the one worth reporting, and this must not replace it.
+          logger.traceException(e);
+        }
+      }
+    }
   }
 
   private boolean isBackendIncomplete(AccessMode accessMode)
@@ -1174,11 +1227,6 @@ private void open0(final Configuration dbCfg) throws ConfigException
     setupStorageFiles(backendDirectory, config.getDBDirectoryPermissions(), config.dn());
     try
     {
-      if (db != null)
-      {
-        throw new IllegalStateException(
-            "Database is already open, either the backend is enabled or an import is currently running.");
-      }
       db = new Persistit(dbCfg);
 
       final long bufferCount = getBufferPoolCfg(dbCfg).computeBufferCount(db.getAvailableHeap());
@@ -1217,7 +1265,8 @@ public <T> T read(final ReadOperation<T> operation) throws Exception
   @Override
   public Importer startImport() throws ConfigException, StorageRuntimeException
   {
-    open0(buildImportConfiguration());
+    rejectIfOpen();
+    openOrGiveBack(buildImportConfiguration());
     return new ImporterImpl();
   }
 

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java

@@ -467,15 +467,8 @@ void endSharedAccess()
     this.dn2uri = new DN2URI(getIndexName(REFERRAL_TREE_NAME), this);
     this.state = new State(getIndexName(STATE_TREE_NAME));
 
-    config.addPluggableChangeListener(this);
-
     attributeIndexCfgManager = new AttributeIndexCfgManager();
-    config.addBackendIndexAddListener(attributeIndexCfgManager);
-    config.addBackendIndexDeleteListener(attributeIndexCfgManager);
-
     vlvIndexCfgManager = new VLVIndexCfgManager();
-    config.addBackendVLVIndexAddListener(vlvIndexCfgManager);
-    config.addBackendVLVIndexDeleteListener(vlvIndexCfgManager);
   }
 
   private CryptoSuite newCryptoSuite(boolean confidentiality)
@@ -533,34 +526,54 @@ void open(WriteableTransaction txn, AccessMode accessMode) throws StorageRuntime
 
         CryptoSuite cryptoSuite = newCryptoSuite(indexCfg.isConfidentialityEnabled());
         final AttributeIndex index = newAttributeIndex(indexCfg, cryptoSuite);
+        // Held before it is opened, because open() is what registers it as a listener of its own
+        // configuration and close() is what takes that off again: an index which fails while
+        // opening is one this container must still be able to close.
+        attrIndexMap.put(indexCfg.getAttribute(), index);
+        attrCryptoMap.put(indexCfg.getAttribute(), cryptoSuite);
         index.open(txn, shouldCreate);
         if(!index.isTrusted() && isNotEmpty)
         {
           logger.info(NOTE_INDEX_ADD_REQUIRES_REBUILD, index.getName());
         }
-        attrIndexMap.put(indexCfg.getAttribute(), index);
-        attrCryptoMap.put(indexCfg.getAttribute(), cryptoSuite);
       }
 
       for (String idx : config.listBackendVLVIndexes())
       {
         BackendVLVIndexCfg vlvIndexCfg = config.getBackendVLVIndex(idx);
 
         VLVIndex vlvIndex = new VLVIndex(vlvIndexCfg, state, storage, this, txn);
+        // Held before it is opened, for the reason given above, and here the window is wider still:
+        // a VLV index registers itself as a listener of its configuration from its constructor.
+        vlvIndexMap.put(vlvIndexCfg.getName().toLowerCase(), vlvIndex);
         vlvIndex.open(txn, shouldCreate);
         if(!vlvIndex.isTrusted() && isNotEmpty)
         {
           logger.info(NOTE_INDEX_ADD_REQUIRES_REBUILD, vlvIndex.getName());
         }
-
-        vlvIndexMap.put(vlvIndexCfg.getName().toLowerCase(), vlvIndex);
       }
+
+      // Registered once everything they answer for is open, and never from the constructor: an
+      // entry container which fails to open is registered nowhere - RootContainer.openEntryContainer
+      // and BackendImpl.changeBaseDNTrees both let the failure through before anything holds it -
+      // so nothing would ever call the close() which takes these off again, and they would go on
+      // answering configuration changes for a backend which is not running. Nothing can reach this
+      // container in between either: open() is called before it is registered anywhere.
+      config.addPluggableChangeListener(this);
+      config.addBackendIndexAddListener(attributeIndexCfgManager);
+      config.addBackendIndexDeleteListener(attributeIndexCfgManager);
+      config.addBackendVLVIndexAddListener(vlvIndexCfgManager);
+      config.addBackendVLVIndexDeleteListener(vlvIndexCfgManager);
     }
-    catch (StorageRuntimeException de)
+    catch (Exception e)
     {
-      logger.traceException(de);
+      // Every failure, not the storage ones alone: open() is declared to throw ConfigException and
+      // does - an index type the attribute has no matching rule for, an index protecting both its
+      // keys and its values, a VLV filter or sort order which does not parse - and the indexes
+      // opened before it registered listeners of their own, which only close() takes back.
+      logger.traceException(e);
       close();
-      throw de;
+      throw e;
     }
   }
 

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java

@@ -17,6 +17,7 @@
  */
 package org.opends.server.backends.pluggable;
 
+import static org.forgerock.util.Utils.closeSilently;
 import static org.opends.messages.BackendMessages.*;
 import static org.opends.server.util.StaticUtils.*;
 
@@ -129,9 +130,12 @@ Storage getStorage()
    */
   void open(final AccessMode accessMode) throws StorageRuntimeException, ConfigException
   {
+    boolean opened = false;
+    boolean storageOpened = false;
     try
     {
       storage.open(accessMode);
+      storageOpened = true;
       storage.write(new WriteOperation()
       {
         @Override
@@ -144,6 +148,7 @@ public void run(WriteableTransaction txn) throws Exception
       // after the write, never inside it: a compressed schema migration is only worth reporting
       // once the transaction that copied it has committed, and a replayed operation runs twice
       compressedSchema.reportMigration();
+      opened = true;
     }
     catch(StorageRuntimeException e)
     {
@@ -153,6 +158,52 @@ public void run(WriteableTransaction txn) throws Exception
     {
       throw new StorageRuntimeException(e);
     }
+    finally
+    {
+      if (!opened)
+      {
+        giveUpAfterFailedOpen(storageOpened);
+      }
+    }
+  }
+
+  /**
+   * Gives back what a root container which failed to open took. Nothing else will: the caller
+   * throws it away - {@link BackendImpl#newRootContainer} lets every failure through without a
+   * reference to it left anywhere - and {@code BackendConfigManager} releases the backend's shared
+   * lock without calling {@code closeBackend()} for a backend which never opened. What is left here
+   * is left for the life of the JVM: entry containers and this root container go on answering the
+   * configuration changes of a backend which is not running, and every later attempt to enable that
+   * backend adds another set of them.
+   * <p>
+   * The failure being given up after is the one worth reporting, so nothing here is allowed to
+   * replace it.
+   *
+   * @param storageOpened whether the storage opened, which is false on one road only: its
+   *          {@code open()} threw. A storage whose open failed is not one this container can
+   *          close - what that open took before it failed is the storage's own to give back, as
+   *          {@code PDBStorage.open} and {@code JDBCStorage.open} do - and there is no other: every
+   *          root container is opened over a storage no root container holds, since
+   *          {@code BackendImpl} opens one, read only or not, only while it has none.
+   */
+  private void giveUpAfterFailedOpen(boolean storageOpened)
+  {
+    try
+    {
+      for (DN baseDN : entryContainers.keySet())
+      {
+        closeSilently(unregisterEntryContainer(baseDN));
+      }
+      config.removePluggableChangeListener(this);
+      if (storageOpened)
+      {
+        storage.close();
+      }
+    }
+    catch (Exception e)
+    {
+      logger.traceException(e);
+    }
   }
 
   /**
@@ -221,12 +272,28 @@ void registerEntryContainer(DN baseDN, EntryContainer entryContainer) throws Ini
   private void openAndRegisterEntryContainers(WriteableTransaction txn, Set<DN> baseDNs, AccessMode accessMode)
       throws StorageRuntimeException, InitializationException, ConfigException
   {
+    // Give up what a previous, rolled back attempt registered: this runs inside the write
+    // Storage.write may replay, and an entry container left registered fails the attempt which
+    // replaces it with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED - so a write-write conflict, which
+    // the replay is there to absorb, would leave the backend unopened instead - while keeping the
+    // configuration listeners it opened with, which only its close() takes back. The same shape as
+    // BackendImpl.changeBaseDNTrees, which opens its entry containers inside a write for the same
+    // reason. Nothing else can reach these containers: the backend registers its base DNs only once
+    // this has returned, so they are closed without being locked.
+    for (DN baseDN : baseDNs)
+    {
+      closeSilently(unregisterEntryContainer(baseDN));
+    }
+
     EntryID highestID = null;
     for (DN baseDN : baseDNs)
     {
       EntryContainer ec = openEntryContainer(baseDN, txn, accessMode);
-      EntryID id = ec.getHighestEntryID(txn);
+      // Registered before anything else here can throw: a container which opened has registered
+      // every listener it ever will, and only what this map holds is given back when the open of
+      // the root container fails.
       registerEntryContainer(baseDN, ec);
+      EntryID id = ec.getHighestEntryID(txn);
       if (highestID == null || id.compareTo(highestID) > 0)
       {
         highestID = id;

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java

@@ -32,6 +32,7 @@
 import org.opends.server.backends.pluggable.spi.AccessMode;
 import org.opends.server.backends.pluggable.spi.ReadOperation;
 import org.opends.server.backends.pluggable.spi.ReadableTransaction;
+import org.opends.server.backends.pluggable.spi.StorageInUseException;
 import org.opends.server.backends.pluggable.spi.StorageRuntimeException;
 import org.opends.server.backends.pluggable.spi.TreeName;
 import org.opends.server.backends.pluggable.spi.WriteOperation;
@@ -422,6 +423,86 @@ public void testRetryDelayGrowsAndStaysBounded()
     assertThat(grown).as("the last attempts still sleep within the first attempt's bound").isGreaterThan(500);
   }
 
+  /**
+   * An open which fails gives back what it took before it failed: the memory it reserved for the
+   * cache, and the listener the constructor registered on the backend configuration. Nothing else
+   * will - a root container does not close a storage which did not open - and a backend whose
+   * volume another storage holds is enabled again and again, each attempt draining one cache size.
+   */
+  @Test
+  public void aStorageWhoseOpenFailedGivesBackWhatItTook() throws Exception
+  {
+    final PDBBackendCfg cfg = createBackendCfg();
+    // Over the volume the storage of setUp() holds: what a second attempt to enable the backend meets.
+    final PDBStorage second = new PDBStorage(cfg, serverContext);
+    final MemoryQuota quota = serverContext.getMemoryQuota();
+    final long availableBefore = quota.getAvailableMemory();
+    try
+    {
+      second.open(AccessMode.READ_WRITE);
+      fail("the storage was expected not to open over a volume another storage holds");
+    }
+    catch (StorageInUseException expected)
+    {
+      // What the lock on the volume file does.
+    }
+
+    assertThat(quota.getAvailableMemory()).isEqualTo(availableBefore);
+    verify(cfg).removePDBChangeListener(second);
+  }
+
+  /**
+   * A storage whose open failed has given everything back already, so closing it afterwards takes
+   * nothing more - {@code BackendImpl.importLDIF} closes the storage of its root container however
+   * the import ended - and does not fail on what the open never got to.
+   */
+  @Test
+  public void closingAStorageWhoseOpenFailedTakesNothingMore() throws Exception
+  {
+    final PDBStorage second = new PDBStorage(createBackendCfg(), serverContext);
+    final MemoryQuota quota = serverContext.getMemoryQuota();
+    final long availableBefore = quota.getAvailableMemory();
+    try
+    {
+      second.open(AccessMode.READ_WRITE);
+      fail("the storage was expected not to open over a volume another storage holds");
+    }
+    catch (StorageInUseException expected)
+    {
+      // What the lock on the volume file does.
+    }
+
+    second.close();
+
+    assertThat(quota.getAvailableMemory()).isEqualTo(availableBefore);
+  }
+
+  /**
+   * A storage which is open refuses to open again before it takes anything, and what it holds is
+   * left as it is: the refusal is a guard against a programming error, not a failed open with
+   * something to give back.
+   */
+  @Test
+  public void openingAnOpenStorageIsRefusedAndTakesNothing() throws Exception
+  {
+    createTree();
+    final MemoryQuota quota = serverContext.getMemoryQuota();
+    final long availableBefore = quota.getAvailableMemory();
+    try
+    {
+      storage.open(AccessMode.READ_WRITE);
+      fail("a storage which is open was expected to refuse to open again");
+    }
+    catch (IllegalStateException expected)
+    {
+      // The guard against a double open.
+    }
+
+    assertThat(quota.getAvailableMemory()).isEqualTo(availableBefore);
+    // Still open: a read reaches the database.
+    assertThat(read("missing")).isNull();
+  }
+
   private void createTree() throws Exception
   {
     storage.write(new WriteOperation()