From cdefc00189e6b3f1fc0a39babe5080fc4d096366 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 9 Sep 2026 16:58:34 +0300 Subject: [PATCH 1/3] [#993] Register an entry container's configuration listeners only once it has opened EntryContainer registered itself and its two configuration managers from its constructor, and only close() takes them off again. open() caught StorageRuntimeException alone, so a ConfigException - a VLV filter or sort order which does not parse, an index type the attribute has no matching rule for - left a container nothing holds a reference to, registered on the configuration of a backend which did not start. Register the five at the end of a successful open() instead, and catch every failure there rather than the storage ones alone. Hold each index in its map before opening it: an attribute index registers its listener at the end of open() and a VLV index from its constructor, so the one being opened was not yet one close() could find - the hole the existing catch already had. RootContainer gives back what a failed open took: the entry containers it registered, its own listener and the storage, the last only when this call is what opened it. openAndRegisterEntryContainers runs inside a write the storage may replay, so it now gives up what a rolled back attempt registered before opening again; without it a write-write conflict failed the backend with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED. Fixes #993 --- .../backends/pluggable/EntryContainer.java | 41 +- .../backends/pluggable/RootContainer.java | 62 ++ .../pluggable/FailedBackendOpenTest.java | 640 ++++++++++++++++++ 3 files changed, 729 insertions(+), 14 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index 917b23009e..d47e2dbbb2 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/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,13 +526,16 @@ 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()) @@ -547,20 +543,37 @@ void open(WriteableTransaction txn, AccessMode accessMode) throws StorageRuntime 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; } } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java index ad1dcd6380..36ac29eeee 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java +++ b/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,50 @@ 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. + *

+ * The failure being given up after is the one worth reporting, so nothing here is allowed to + * replace it. + * + * @param storageOpened whether this call is the one which opened the storage. A read only root + * container is opened over the very storage instance the backend holds - see + * {@code BackendImpl.getReadOnlyRootContainer} - and closing one it did not open would + * take the volume from under the root container which does hold it. + */ + 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,6 +270,19 @@ void registerEntryContainer(DN baseDN, EntryContainer entryContainer) throws Ini private void openAndRegisterEntryContainers(WriteableTransaction txn, Set 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) { diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java new file mode 100644 index 0000000000..3c08e8cabf --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java @@ -0,0 +1,640 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.opends.server.backends.pluggable; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.forgerock.opendj.config.ConfigurationMock.mockCfg; +import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.opends.server.util.CollectionUtils.newTreeSet; +import static org.testng.Assert.fail; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.SortedSet; + +import org.forgerock.opendj.config.server.ConfigException; +import org.forgerock.opendj.config.server.ConfigurationAddListener; +import org.forgerock.opendj.config.server.ConfigurationChangeListener; +import org.forgerock.opendj.config.server.ConfigurationDeleteListener; +import org.forgerock.opendj.ldap.ByteSequence; +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; +import org.forgerock.opendj.ldap.schema.AttributeType; +import org.forgerock.opendj.server.config.meta.BackendIndexCfgDefn.IndexType; +import org.forgerock.opendj.server.config.meta.BackendVLVIndexCfgDefn.Scope; +import org.forgerock.opendj.server.config.server.BackendIndexCfg; +import org.forgerock.opendj.server.config.server.BackendVLVIndexCfg; +import org.forgerock.opendj.server.config.server.PDBBackendCfg; +import org.forgerock.opendj.server.config.server.PluggableBackendCfg; +import org.mockito.ArgumentCaptor; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.backends.pdb.PDBStorage; +import org.opends.server.backends.pluggable.spi.AccessMode; +import org.opends.server.backends.pluggable.spi.Cursor; +import org.opends.server.backends.pluggable.spi.Importer; +import org.opends.server.backends.pluggable.spi.ReadOperation; +import org.opends.server.backends.pluggable.spi.Storage; +import org.opends.server.backends.pluggable.spi.StorageInUseException; +import org.opends.server.backends.pluggable.spi.StorageRuntimeException; +import org.opends.server.backends.pluggable.spi.StorageStatus; +import org.opends.server.backends.pluggable.spi.TreeName; +import org.opends.server.backends.pluggable.spi.UpdateFunction; +import org.opends.server.backends.pluggable.spi.WriteOperation; +import org.opends.server.backends.pluggable.spi.WriteableTransaction; +import org.opends.server.core.ServerContext; +import org.opends.server.types.BackupConfig; +import org.opends.server.types.BackupDirectory; +import org.opends.server.types.DirectoryException; +import org.opends.server.types.InitializationException; +import org.opends.server.types.RestoreConfig; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import com.persistit.exception.RollbackException; + +/** + * Tests that a backend which fails to open gives back everything its opening took - see OpenDJ + * issue #993. + *

+ * {@link EntryContainer} registers itself and its two configuration managers as listeners of the + * backend configuration, and every index it opens registers one of its own. Only + * {@link EntryContainer#close()} takes them off again, and an entry container whose + * {@link EntryContainer#open} failed is registered nowhere, so nothing will ever call it: the + * listeners of a backend which is not running answer configuration changes for the life of the JVM. + */ +@SuppressWarnings("javadoc") +@Test(groups = { "precommit", "pluggablebackend" }, sequential = true) +public class FailedBackendOpenTest extends DirectoryServerTestCase +{ + private static final String BACKEND_ID = "FailedBackendOpenTest"; + private static final DN BASE_DN = DN.valueOf("dc=b993,dc=com"); + + private ServerContext serverContext; + private AttributeType cnType; + /** The index configuration the entry container's attribute index registers with. */ + private BackendIndexCfg indexCfg; + /** The VLV index configuration the entry container's VLV index registers with. */ + private BackendVLVIndexCfg vlvIndexCfg; + + @BeforeClass + public void startServer() throws Exception + { + TestCaseUtils.startServer(); + serverContext = TestCaseUtils.getServerContext(); + cnType = serverContext.getSchema().getAttributeType("cn"); + } + + /** + * A VLV index whose filter does not parse fails {@code EntryContainer.open()} with a + * {@link ConfigException}, after the attribute indexes ahead of it have opened and registered + * their own listeners. The backend does not open, and nothing it registered may be left behind. + */ + @Test + public void aBackendWhichFailsToOpenLeavesNothingRegistered() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + when(vlvIndexCfg.getFilter()).thenReturn("(&(objectClass=*)"); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + try + { + backend.openBackend(); + fail("the backend was expected not to open with a VLV index whose filter does not parse"); + } + catch (InitializationException expected) + { + // What an index the stored configuration names, and the schema no longer supports, does. + } + finally + { + backend.storage.close(); + } + + assertThat(stillRegisteredOn(cfg)).isEmpty(); + assertThat(stillRegisteredOn(indexCfg)).isEmpty(); + } + + /** + * A VLV index registers itself as a listener of its configuration from its constructor, and only + * reaches the entry container's map once it has opened. A failure in between is the one the + * container has always caught, and the index it is closing is still not one it holds. + */ + @Test + public void anIndexWhichFailsToOpenLeavesNoListenerBehind() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + backend.storage.failOpeningTree("vlv.vlv1"); + try + { + backend.openBackend(); + fail("the backend was expected not to open with a VLV index whose tree cannot be opened"); + } + catch (InitializationException expected) + { + // What a storage which cannot give the index its tree does. + } + finally + { + backend.storage.close(); + } + + assertThat(stillRegisteredOn(cfg)).isEmpty(); + assertThat(stillRegisteredOn(vlvIndexCfg)).isEmpty(); + } + + /** + * The storage a failed open opened is given back along with the listeners. + * {@code BackendConfigManager} releases the backend's shared lock and never calls + * {@code closeBackend()} for a backend which did not open, so a volume left open here is one no + * later attempt to enable that backend can take. + */ + @Test + public void aBackendWhichFailsToOpenGivesBackTheStorageItOpened() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + when(vlvIndexCfg.getFilter()).thenReturn("(&(objectClass=*)"); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + final int closesAfterTheFailure; + try + { + try + { + backend.openBackend(); + fail("the backend was expected not to open with a VLV index whose filter does not parse"); + } + catch (InitializationException expected) + { + // What an index the stored configuration names, and the schema no longer supports, does. + } + closesAfterTheFailure = backend.storage.closeCalls(); + } + finally + { + // A no-op once the failed open has given the storage back, and what keeps the tests which + // follow runnable if it has not. + backend.storage.close(); + } + + assertThat(closesAfterTheFailure).isEqualTo(1); + } + + /** + * A root container which could not open the storage has not got it to give back: a read only + * root container is opened over the very storage instance the backend holds, and closing that + * one would take the volume from under the root container which does hold it. + */ + @Test + public void aRootContainerWhichCouldNotOpenTheStorageDoesNotCloseIt() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + backend.storage.refuseToOpen(); + try + { + backend.openBackend(); + fail("the backend was expected not to open over a storage which is already held"); + } + catch (InitializationException expected) + { + // What a storage another root container holds does. + } + + assertThat(backend.storage.closeCalls()).isEqualTo(0); + } + + /** + * {@code RootContainer.open} opens and registers its entry containers inside the write a storage + * may replay after a transaction conflict. The attempt which replaces a rolled back one must find + * the registry as the first one found it: an entry container left registered fails it with + * {@code ERR_ENTRY_CONTAINER_ALREADY_REGISTERED}, so an ordinary write-write conflict becomes a + * backend which does not start, and the container it left behind keeps the listeners it opened + * with. + */ + @Test + public void aReplayedOpenLeavesOneSetOfEntryContainers() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + backend.storage.conflictAtCommit(1); + try + { + backend.openBackend(); + } + catch (Exception failedToOpen) + { + // Leave the volume closed, or every test which follows fails in openBackend() too and the + // one which actually broke is lost among them. + backend.storage.close(); + throw failedToOpen; + } + try + { + assertThat(backend.storage.writeAttempts()).isEqualTo(2); + assertThat(backend.getRootContainer().getBaseDNs()).containsOnly(BASE_DN); + } + finally + { + backend.finalizeBackend(); + } + + // Closing the backend takes back what the backend which is running registered, so anything + // still registered here belongs to the attempt which was rolled back. + assertThat(stillRegisteredOn(cfg)).isEmpty(); + assertThat(stillRegisteredOn(indexCfg)).isEmpty(); + assertThat(stillRegisteredOn(vlvIndexCfg)).isEmpty(); + } + + /** Every listener added to the backend configuration and not taken off it again. */ + private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Exception + { + final List registered = new ArrayList<>(); + + final ArgumentCaptor> changeAdded = + captorFor(ConfigurationChangeListener.class); + verify(cfg, atLeast(0)).addPluggableChangeListener(changeAdded.capture()); + registered.addAll(changeAdded.getAllValues()); + final ArgumentCaptor> changeRemoved = + captorFor(ConfigurationChangeListener.class); + verify(cfg, atLeast(0)).removePluggableChangeListener(changeRemoved.capture()); + registered.removeAll(changeRemoved.getAllValues()); + + final ArgumentCaptor> indexAdded = + captorFor(ConfigurationAddListener.class); + verify(cfg, atLeast(0)).addBackendIndexAddListener(indexAdded.capture()); + registered.addAll(indexAdded.getAllValues()); + final ArgumentCaptor> indexAddRemoved = + captorFor(ConfigurationAddListener.class); + verify(cfg, atLeast(0)).removeBackendIndexAddListener(indexAddRemoved.capture()); + registered.removeAll(indexAddRemoved.getAllValues()); + + final ArgumentCaptor> indexDeleteAdded = + captorFor(ConfigurationDeleteListener.class); + verify(cfg, atLeast(0)).addBackendIndexDeleteListener(indexDeleteAdded.capture()); + registered.addAll(indexDeleteAdded.getAllValues()); + final ArgumentCaptor> indexDeleteRemoved = + captorFor(ConfigurationDeleteListener.class); + verify(cfg, atLeast(0)).removeBackendIndexDeleteListener(indexDeleteRemoved.capture()); + registered.removeAll(indexDeleteRemoved.getAllValues()); + + final ArgumentCaptor> vlvAdded = + captorFor(ConfigurationAddListener.class); + verify(cfg, atLeast(0)).addBackendVLVIndexAddListener(vlvAdded.capture()); + registered.addAll(vlvAdded.getAllValues()); + final ArgumentCaptor> vlvAddRemoved = + captorFor(ConfigurationAddListener.class); + verify(cfg, atLeast(0)).removeBackendVLVIndexAddListener(vlvAddRemoved.capture()); + registered.removeAll(vlvAddRemoved.getAllValues()); + + final ArgumentCaptor> vlvDeleteAdded = + captorFor(ConfigurationDeleteListener.class); + verify(cfg, atLeast(0)).addBackendVLVIndexDeleteListener(vlvDeleteAdded.capture()); + registered.addAll(vlvDeleteAdded.getAllValues()); + final ArgumentCaptor> vlvDeleteRemoved = + captorFor(ConfigurationDeleteListener.class); + verify(cfg, atLeast(0)).removeBackendVLVIndexDeleteListener(vlvDeleteRemoved.capture()); + registered.removeAll(vlvDeleteRemoved.getAllValues()); + + return registered; + } + + /** Every listener added to an index configuration and not taken off it again. */ + private static List stillRegisteredOn(BackendIndexCfg cfg) + { + final List registered = new ArrayList<>(); + final ArgumentCaptor> added = + captorFor(ConfigurationChangeListener.class); + verify(cfg, atLeast(0)).addChangeListener(added.capture()); + registered.addAll(added.getAllValues()); + final ArgumentCaptor> removed = + captorFor(ConfigurationChangeListener.class); + verify(cfg, atLeast(0)).removeChangeListener(removed.capture()); + registered.removeAll(removed.getAllValues()); + return registered; + } + + /** Every listener added to a VLV index configuration and not taken off it again. */ + private static List stillRegisteredOn(BackendVLVIndexCfg cfg) + { + final List registered = new ArrayList<>(); + final ArgumentCaptor> added = + captorFor(ConfigurationChangeListener.class); + verify(cfg, atLeast(0)).addChangeListener(added.capture()); + registered.addAll(added.getAllValues()); + final ArgumentCaptor> removed = + captorFor(ConfigurationChangeListener.class); + verify(cfg, atLeast(0)).removeChangeListener(removed.capture()); + registered.removeAll(removed.getAllValues()); + return registered; + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + private static ArgumentCaptor captorFor(Class listenerClass) + { + return (ArgumentCaptor) ArgumentCaptor.forClass((Class) listenerClass); + } + + private PDBBackendCfg backendCfg(SortedSet baseDNs) throws ConfigException + { + final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); + when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); + when(cfg.getBackendId()).thenReturn(BACKEND_ID); + when(cfg.getDBDirectory()).thenReturn(BACKEND_ID); + when(cfg.getDBDirectoryPermissions()).thenReturn("755"); + when(cfg.getDBCacheSize()).thenReturn(0L); + when(cfg.getDBCachePercent()).thenReturn(20); + when(cfg.getBaseDN()).thenReturn(baseDNs); + when(cfg.listBackendIndexes()).thenReturn(new String[] { "cn" }); + when(cfg.listBackendVLVIndexes()).thenReturn(new String[] { "vlv1" }); + + indexCfg = mock(BackendIndexCfg.class); + when(indexCfg.getIndexType()).thenReturn(newTreeSet(IndexType.EQUALITY)); + when(indexCfg.getAttribute()).thenReturn(cnType); + when(indexCfg.getIndexEntryLimit()).thenReturn(4000); + when(indexCfg.getSubstringLength()).thenReturn(6); + when(cfg.getBackendIndex("cn")).thenReturn(indexCfg); + + vlvIndexCfg = mock(BackendVLVIndexCfg.class); + when(vlvIndexCfg.getName()).thenReturn("vlv1"); + when(vlvIndexCfg.getBaseDN()).thenReturn(baseDNs.first()); + when(vlvIndexCfg.getScope()).thenReturn(Scope.WHOLE_SUBTREE); + when(vlvIndexCfg.getFilter()).thenReturn("(objectClass=*)"); + when(vlvIndexCfg.getSortOrder()).thenReturn("+cn"); + when(cfg.getBackendVLVIndex("vlv1")).thenReturn(vlvIndexCfg); + return cfg; + } + + /** A backend whose storage the test can reach, and which counts what closes it. */ + private static final class TrackedBackend extends BackendImpl + { + private TrackingStorage storage; + + @Override + protected Storage configureStorage(PDBBackendCfg cfg, ServerContext serverContext) throws ConfigException + { + storage = new TrackingStorage(new PDBStorage(cfg, serverContext)); + return storage; + } + } + + /** + * Decorates a {@link Storage} so that the test can tell whether what opened it also gave it back. + * {@link #close()} is answered once for each time the storage was opened, so that a test can + * close what a failure left open without hiding whether it had already been closed. + */ + private static final class TrackingStorage implements Storage + { + private final Storage delegate; + private boolean open; + private int closeCalls; + /** The index id of the tree no transaction of this storage will open, if any. */ + private String failingTreeId; + /** Whether this storage refuses to open at all, as one another root container holds does. */ + private boolean refuseToOpen; + /** How many write operations are still to be conflicted once they have run. */ + private int conflictsLeft; + private int writeAttempts; + + TrackingStorage(Storage delegate) + { + this.delegate = delegate; + } + + /** How many times this storage was asked to close, whether it was open or not. */ + int closeCalls() + { + return closeCalls; + } + + /** Makes every transaction of this storage refuse to open the named tree. */ + void failOpeningTree(String treeId) + { + failingTreeId = treeId; + } + + /** Makes this storage refuse to open, as one another root container already holds does. */ + void refuseToOpen() + { + refuseToOpen = true; + } + + /** + * Makes the next write operations conflict once they have run, as PersistIt reports a + * write-write conflict at commit time. The conflict is raised from within the single write the + * delegate is asked for, so the replay is the delegate's own retry loop. + */ + void conflictAtCommit(int conflicts) + { + conflictsLeft = conflicts; + } + + /** How many times a write operation was run, the replays included. */ + int writeAttempts() + { + return writeAttempts; + } + + @Override + public void open(AccessMode accessMode) throws Exception + { + if (refuseToOpen) + { + throw new StorageInUseException("held by another root container"); + } + delegate.open(accessMode); + open = true; + } + + @Override + public void close() + { + closeCalls++; + // Only what was opened is given back, so that a test can close what a failure left open + // without closing the delegate twice. + if (open) + { + open = false; + delegate.close(); + } + } + + @Override + public void write(final WriteOperation writeOperation) throws Exception + { + delegate.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + writeAttempts++; + writeOperation.run(failingTreeId != null ? new RefusingOneTree(txn, failingTreeId) : txn); + if (conflictsLeft > 0) + { + conflictsLeft--; + throw new RollbackException(); + } + } + }); + } + + @Override + public T read(ReadOperation readOperation) throws Exception + { + return delegate.read(readOperation); + } + + @Override + public Importer startImport() throws ConfigException + { + return delegate.startImport(); + } + + @Override + public void removeStorageFiles() + { + delegate.removeStorageFiles(); + } + + @Override + public StorageStatus getStorageStatus() + { + return delegate.getStorageStatus(); + } + + @Override + public Set listTrees() + { + return delegate.listTrees(); + } + + @Override + public boolean supportsBackupAndRestore() + { + return delegate.supportsBackupAndRestore(); + } + + @Override + public void createBackup(BackupConfig backupConfig) throws DirectoryException + { + delegate.createBackup(backupConfig); + } + + @Override + public void removeBackup(BackupDirectory backupDirectory, String backupID) throws DirectoryException + { + delegate.removeBackup(backupDirectory, backupID); + } + + @Override + public void restoreBackup(RestoreConfig restoreConfig) throws DirectoryException + { + delegate.restoreBackup(restoreConfig); + } + } + + /** + * A transaction which is every bit the one it decorates, except that it will not open one named + * tree - what a storage which cannot give an index the tree it asks for does. + */ + private static final class RefusingOneTree implements WriteableTransaction + { + private final WriteableTransaction delegate; + private final String refused; + + RefusingOneTree(WriteableTransaction delegate, String refused) + { + this.delegate = delegate; + this.refused = refused; + } + + @Override + public void openTree(TreeName name, boolean createOnDemand) + { + if (refused.equals(name.getIndexId())) + { + throw new StorageRuntimeException("cannot open " + name); + } + delegate.openTree(name, createOnDemand); + } + + @Override + public void deleteTree(TreeName name) + { + delegate.deleteTree(name); + } + + @Override + public void put(TreeName treeName, ByteSequence key, ByteSequence value) + { + delegate.put(treeName, key, value); + } + + @Override + public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) + { + return delegate.update(treeName, key, f); + } + + @Override + public boolean delete(TreeName treeName, ByteSequence key) + { + return delegate.delete(treeName, key); + } + + @Override + public ByteString read(TreeName treeName, ByteSequence key) + { + return delegate.read(treeName, key); + } + + @Override + public Cursor openCursor(TreeName treeName) + { + return delegate.openCursor(treeName); + } + + @Override + public long getRecordCount(TreeName treeName) + { + return delegate.getRecordCount(treeName); + } + + @Override + public boolean treeExists(TreeName treeName) + { + return delegate.treeExists(treeName); + } + } +} From 42f6aa2a7217c57ea2145bb779d01b762d7f884a Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 16 Sep 2026 17:53:02 +0300 Subject: [PATCH 2/3] [#993] Give back what a failed PDBStorage open took, and pin the give-back of the root container Review round 2 of #999. PDBStorage.open() and startImport() give back what the attempt took before it failed - the cache size buildConfiguration drew from the memory quota, the listener the constructor registered on the backend configuration, and the database when the open got that far - as JDBCStorage.open already does. Every failed enable of a PDB backend drained one cache size for the life of the JVM and left a storage answering the configuration changes of a backend which is not running. The guard against a double open runs before anything is taken, and close() releases the quota once and tolerates a database the failed open registered no monitor for. RootContainer registers an entry container as soon as it has opened, before its highest entry ID is read: a container which opened has registered every listener it ever will, and only what the registry holds is given back. The rationale of the storageOpened arm is the true one: the storage's own open() threw, and what that open took is the storage's own to give back - no root container is ever opened over a storage another one holds. FailedBackendOpenTest pins the give-back loop with a second base DN, the registration of an opened container, the root container's own listener when the storage did not open, and the positive twin - one registration of each listener once a container has opened, counted per occurrence. PDBStorageTest pins the give-back, a close() which follows it, and the refusal of a double open. --- .../server/backends/pdb/PDBStorage.java | 67 +++- .../backends/pluggable/RootContainer.java | 15 +- .../server/backends/pdb/PDBStorageTest.java | 81 ++++ .../pluggable/FailedBackendOpenTest.java | 348 +++++++++++++----- 4 files changed, 414 insertions(+), 97 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java index 7ad8cc764b..5ca9d4744d 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java +++ b/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 read(final ReadOperation operation) throws Exception @Override public Importer startImport() throws ConfigException, StorageRuntimeException { - open0(buildImportConfiguration()); + rejectIfOpen(); + openOrGiveBack(buildImportConfiguration()); return new ImporterImpl(); } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java index 36ac29eeee..7d6270fa35 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java @@ -179,10 +179,12 @@ public void run(WriteableTransaction txn) throws Exception * The failure being given up after is the one worth reporting, so nothing here is allowed to * replace it. * - * @param storageOpened whether this call is the one which opened the storage. A read only root - * container is opened over the very storage instance the backend holds - see - * {@code BackendImpl.getReadOnlyRootContainer} - and closing one it did not open would - * take the volume from under the root container which does hold 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) { @@ -287,8 +289,11 @@ private void openAndRegisterEntryContainers(WriteableTransaction txn, Set ba 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; diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java index f633849bb5..8780aee0ab 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java +++ b/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() diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java index 3c08e8cabf..d66fc56d4a 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java @@ -25,9 +25,11 @@ import static org.testng.Assert.fail; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.SortedSet; +import java.util.function.BooleanSupplier; import org.forgerock.opendj.config.server.ConfigException; import org.forgerock.opendj.config.server.ConfigurationAddListener; @@ -86,6 +88,8 @@ public class FailedBackendOpenTest extends DirectoryServerTestCase { private static final String BACKEND_ID = "FailedBackendOpenTest"; private static final DN BASE_DN = DN.valueOf("dc=b993,dc=com"); + /** A second base DN of the same backend, sorting after {@link #BASE_DN}. */ + private static final DN SECOND_BASE_DN = DN.valueOf("dc=b993b,dc=com"); private ServerContext serverContext; private AttributeType cnType; @@ -147,7 +151,7 @@ public void anIndexWhichFailsToOpenLeavesNoListenerBehind() throws Exception final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); - backend.storage.failOpeningTree("vlv.vlv1"); + backend.storage.failOpeningTree("/dc=com,dc=b993/vlv.vlv1"); try { backend.openBackend(); @@ -206,9 +210,10 @@ public void aBackendWhichFailsToOpenGivesBackTheStorageItOpened() throws Excepti } /** - * A root container which could not open the storage has not got it to give back: a read only - * root container is opened over the very storage instance the backend holds, and closing that - * one would take the volume from under the root container which does hold it. + * A root container whose storage would not open has nothing of the storage's to give back: a + * {@code Storage.open()} which threw returns what it took itself, and a storage which never + * opened is not one to close. What is the root container's own - the listener it registered from + * its constructor - it gives back all the same. */ @Test public void aRootContainerWhichCouldNotOpenTheStorageDoesNotCloseIt() throws Exception @@ -222,14 +227,129 @@ public void aRootContainerWhichCouldNotOpenTheStorageDoesNotCloseIt() throws Exc try { backend.openBackend(); - fail("the backend was expected not to open over a storage which is already held"); + fail("the backend was expected not to open over a storage whose volume is locked"); } catch (InitializationException expected) { - // What a storage another root container holds does. + // What a storage whose volume another process holds does. } assertThat(backend.storage.closeCalls()).isEqualTo(0); + assertThat(stillRegisteredOn(cfg)).isEmpty(); + } + + /** + * The give-back of a failed open walks the entry containers the attempt had registered, which + * every base DN but the last one leaves behind when a later one fails: the first base DN's + * container is open, registered and answering configuration changes by the time the second one + * cannot open its trees. + */ + @Test + public void aSecondBaseDNWhichFailsToOpenGivesBackTheFirst() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + // A tree set, as the configuration's own is: dc=b993 sorts before dc=b993b, so it is the one + // opened - and registered - first. + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN, SECOND_BASE_DN)); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + backend.storage.failOpeningTree("/dc=com,dc=b993b/id2entry"); + try + { + backend.openBackend(); + fail("the backend was expected not to open with a second base DN whose trees cannot be opened"); + } + catch (InitializationException expected) + { + // What a storage which cannot give the second container its trees does. + } + finally + { + backend.storage.close(); + } + + // The road this test is about: the first container had opened before the second one failed. + assertThat(backend.storage.openedTrees()).contains("/dc=com,dc=b993/id2entry"); + assertThat(stillRegisteredOn(cfg)).isEmpty(); + assertThat(stillRegisteredOn(indexCfg)).isEmpty(); + assertThat(stillRegisteredOn(vlvIndexCfg)).isEmpty(); + } + + /** + * An entry container which opened has registered everything it ever will, and it is registered + * with the root container only after its highest entry ID has been read. A failure of that read + * leaves a container which nothing holds, unless it is registered before anything else can + * throw. + */ + @Test + public void anEntryContainerWhichOpenedButWasNotRegisteredIsGivenBack() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + // The read of the highest entry ID is the first cursor over id2entry once the container has + // opened, which is when it registers itself: every cursor before that - the emptiness check of + // EntryContainer.open() and the one each untrusted index makes as it opens - fails the open + // itself, which the container catches. + backend.storage.failOpeningCursor("/dc=com,dc=b993/id2entry", () -> anEntryContainerIsRegisteredOn(cfg)); + try + { + backend.openBackend(); + fail("the backend was expected not to open when the highest entry ID cannot be read"); + } + catch (InitializationException expected) + { + // What a storage which cannot position a cursor on the last entry does. + } + finally + { + backend.storage.close(); + } + + assertThat(stillRegisteredOn(cfg)).isEmpty(); + assertThat(stillRegisteredOn(indexCfg)).isEmpty(); + assertThat(stillRegisteredOn(vlvIndexCfg)).isEmpty(); + } + + /** + * The positive twin of the tests above: an entry container which opened is registered, once, as + * a listener of the backend configuration, and so are its two configuration managers and each + * index it opened. Without it, the registrations could be dropped and every test of a failed + * open would stay green. + */ + @Test + public void anEntryContainerWhichOpenedIsRegisteredOnce() throws Exception + { + final TrackedBackend backend = new TrackedBackend(); + backend.setBackendID(BACKEND_ID); + final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); + backend.configureBackend(cfg, serverContext); + backend.storage.removeStorageFiles(); + backend.openBackend(); + final List registered; + final List registeredOnIndex; + final List registeredOnVLVIndex; + try + { + registered = stillRegisteredOn(cfg); + registeredOnIndex = stillRegisteredOn(indexCfg); + registeredOnVLVIndex = stillRegisteredOn(vlvIndexCfg); + } + finally + { + backend.finalizeBackend(); + } + + assertThat(registered).filteredOn(listener -> listener instanceof EntryContainer).hasSize(1); + // The two configuration managers, each once as an add listener and once as a delete listener. + assertThat(registered) + .filteredOn(listener -> listener.getClass().getSimpleName().endsWith("IndexCfgManager")) + .hasSize(4); + assertThat(registeredOnIndex).hasSize(1); + assertThat(registeredOnVLVIndex).hasSize(1); } /** @@ -278,7 +398,7 @@ public void aReplayedOpenLeavesOneSetOfEntryContainers() throws Exception } /** Every listener added to the backend configuration and not taken off it again. */ - private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Exception + private static List stillRegisteredOn(PluggableBackendCfg cfg) throws ConfigException { final List registered = new ArrayList<>(); @@ -289,7 +409,7 @@ private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Ex final ArgumentCaptor> changeRemoved = captorFor(ConfigurationChangeListener.class); verify(cfg, atLeast(0)).removePluggableChangeListener(changeRemoved.capture()); - registered.removeAll(changeRemoved.getAllValues()); + removeEach(registered, changeRemoved.getAllValues()); final ArgumentCaptor> indexAdded = captorFor(ConfigurationAddListener.class); @@ -298,7 +418,7 @@ private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Ex final ArgumentCaptor> indexAddRemoved = captorFor(ConfigurationAddListener.class); verify(cfg, atLeast(0)).removeBackendIndexAddListener(indexAddRemoved.capture()); - registered.removeAll(indexAddRemoved.getAllValues()); + removeEach(registered, indexAddRemoved.getAllValues()); final ArgumentCaptor> indexDeleteAdded = captorFor(ConfigurationDeleteListener.class); @@ -307,7 +427,7 @@ private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Ex final ArgumentCaptor> indexDeleteRemoved = captorFor(ConfigurationDeleteListener.class); verify(cfg, atLeast(0)).removeBackendIndexDeleteListener(indexDeleteRemoved.capture()); - registered.removeAll(indexDeleteRemoved.getAllValues()); + removeEach(registered, indexDeleteRemoved.getAllValues()); final ArgumentCaptor> vlvAdded = captorFor(ConfigurationAddListener.class); @@ -316,7 +436,7 @@ private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Ex final ArgumentCaptor> vlvAddRemoved = captorFor(ConfigurationAddListener.class); verify(cfg, atLeast(0)).removeBackendVLVIndexAddListener(vlvAddRemoved.capture()); - registered.removeAll(vlvAddRemoved.getAllValues()); + removeEach(registered, vlvAddRemoved.getAllValues()); final ArgumentCaptor> vlvDeleteAdded = captorFor(ConfigurationDeleteListener.class); @@ -325,7 +445,7 @@ private static List stillRegisteredOn(PluggableBackendCfg cfg) throws Ex final ArgumentCaptor> vlvDeleteRemoved = captorFor(ConfigurationDeleteListener.class); verify(cfg, atLeast(0)).removeBackendVLVIndexDeleteListener(vlvDeleteRemoved.capture()); - registered.removeAll(vlvDeleteRemoved.getAllValues()); + removeEach(registered, vlvDeleteRemoved.getAllValues()); return registered; } @@ -341,7 +461,7 @@ private static List stillRegisteredOn(BackendIndexCfg cfg) final ArgumentCaptor> removed = captorFor(ConfigurationChangeListener.class); verify(cfg, atLeast(0)).removeChangeListener(removed.capture()); - registered.removeAll(removed.getAllValues()); + removeEach(registered, removed.getAllValues()); return registered; } @@ -356,10 +476,45 @@ private static List stillRegisteredOn(BackendVLVIndexCfg cfg) final ArgumentCaptor> removed = captorFor(ConfigurationChangeListener.class); verify(cfg, atLeast(0)).removeChangeListener(removed.capture()); - registered.removeAll(removed.getAllValues()); + removeEach(registered, removed.getAllValues()); return registered; } + /** + * Whether an entry container is registered as a listener of the backend configuration, which is + * the last thing {@code EntryContainer.open()} does. + */ + private static boolean anEntryContainerIsRegisteredOn(PluggableBackendCfg cfg) + { + try + { + for (Object listener : stillRegisteredOn(cfg)) + { + if (listener instanceof EntryContainer) + { + return true; + } + } + return false; + } + catch (ConfigException declaredButNeverThrownByAMock) + { + throw new AssertionError(declaredButNeverThrownByAMock); + } + } + + /** + * Takes one occurrence off the registrations for every removal, rather than every occurrence + * for any: a listener registered twice and taken off once is still registered. + */ + private static void removeEach(List registered, List removed) + { + for (Object listener : removed) + { + registered.remove(listener); + } + } + @SuppressWarnings({ "unchecked", "rawtypes" }) private static ArgumentCaptor captorFor(Class listenerClass) { @@ -419,9 +574,15 @@ private static final class TrackingStorage implements Storage private final Storage delegate; private boolean open; private int closeCalls; - /** The index id of the tree no transaction of this storage will open, if any. */ - private String failingTreeId; - /** Whether this storage refuses to open at all, as one another root container holds does. */ + /** The full name of the tree no transaction of this storage will open, if any. */ + private String failingTree; + /** The full name of the tree one cursor of which no transaction of this storage will open, if any. */ + private String failingCursorTree; + /** Once this holds, the next cursor over {@link #failingCursorTree} is the one refused. */ + private BooleanSupplier cursorRefusalDue; + /** The full names of every tree a transaction of this storage opened. */ + private final Set openedTrees = new HashSet<>(); + /** Whether this storage refuses to open at all, as one whose volume another process holds does. */ private boolean refuseToOpen; /** How many write operations are still to be conflicted once they have run. */ private int conflictsLeft; @@ -438,13 +599,29 @@ int closeCalls() return closeCalls; } - /** Makes every transaction of this storage refuse to open the named tree. */ - void failOpeningTree(String treeId) + /** Makes every transaction of this storage refuse to open the tree of the given full name. */ + void failOpeningTree(String treeName) + { + failingTree = treeName; + } + + /** + * Makes the transactions of this storage refuse to open one cursor over the tree of the given + * full name: the first one asked for once the given condition holds. + */ + void failOpeningCursor(String treeName, BooleanSupplier once) + { + failingCursorTree = treeName; + cursorRefusalDue = once; + } + + /** The full names of every tree a transaction of this storage opened. */ + Set openedTrees() { - failingTreeId = treeId; + return openedTrees; } - /** Makes this storage refuse to open, as one another root container already holds does. */ + /** Makes this storage refuse to open, as one whose volume another process holds does. */ void refuseToOpen() { refuseToOpen = true; @@ -471,7 +648,7 @@ public void open(AccessMode accessMode) throws Exception { if (refuseToOpen) { - throw new StorageInUseException("held by another root container"); + throw new StorageInUseException("the volume is locked by another process"); } delegate.open(accessMode); open = true; @@ -499,7 +676,7 @@ public void write(final WriteOperation writeOperation) throws Exception public void run(WriteableTransaction txn) throws Exception { writeAttempts++; - writeOperation.run(failingTreeId != null ? new RefusingOneTree(txn, failingTreeId) : txn); + writeOperation.run(new TrackingTransaction(txn)); if (conflictsLeft > 0) { conflictsLeft--; @@ -562,79 +739,84 @@ public void restoreBackup(RestoreConfig restoreConfig) throws DirectoryException { delegate.restoreBackup(restoreConfig); } - } - - /** - * A transaction which is every bit the one it decorates, except that it will not open one named - * tree - what a storage which cannot give an index the tree it asks for does. - */ - private static final class RefusingOneTree implements WriteableTransaction - { - private final WriteableTransaction delegate; - private final String refused; - RefusingOneTree(WriteableTransaction delegate, String refused) + /** + * A transaction which is every bit the one it decorates, except that it records the trees it + * opens and refuses what the storage was told to refuse: one named tree - what a storage which + * cannot give an index the tree it asks for does - or one cursor over one. + */ + private final class TrackingTransaction implements WriteableTransaction { - this.delegate = delegate; - this.refused = refused; - } + private final WriteableTransaction delegate; - @Override - public void openTree(TreeName name, boolean createOnDemand) - { - if (refused.equals(name.getIndexId())) + TrackingTransaction(WriteableTransaction delegate) { - throw new StorageRuntimeException("cannot open " + name); + this.delegate = delegate; } - delegate.openTree(name, createOnDemand); - } - @Override - public void deleteTree(TreeName name) - { - delegate.deleteTree(name); - } + @Override + public void openTree(TreeName name, boolean createOnDemand) + { + if (name.toString().equals(failingTree)) + { + throw new StorageRuntimeException("cannot open " + name); + } + delegate.openTree(name, createOnDemand); + openedTrees.add(name.toString()); + } - @Override - public void put(TreeName treeName, ByteSequence key, ByteSequence value) - { - delegate.put(treeName, key, value); - } + @Override + public Cursor openCursor(TreeName treeName) + { + if (treeName.toString().equals(failingCursorTree) && cursorRefusalDue.getAsBoolean()) + { + failingCursorTree = null; + throw new StorageRuntimeException("cannot open a cursor over " + treeName); + } + return delegate.openCursor(treeName); + } - @Override - public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) - { - return delegate.update(treeName, key, f); - } + @Override + public void deleteTree(TreeName name) + { + delegate.deleteTree(name); + } - @Override - public boolean delete(TreeName treeName, ByteSequence key) - { - return delegate.delete(treeName, key); - } + @Override + public void put(TreeName treeName, ByteSequence key, ByteSequence value) + { + delegate.put(treeName, key, value); + } - @Override - public ByteString read(TreeName treeName, ByteSequence key) - { - return delegate.read(treeName, key); - } + @Override + public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) + { + return delegate.update(treeName, key, f); + } - @Override - public Cursor openCursor(TreeName treeName) - { - return delegate.openCursor(treeName); - } + @Override + public boolean delete(TreeName treeName, ByteSequence key) + { + return delegate.delete(treeName, key); + } - @Override - public long getRecordCount(TreeName treeName) - { - return delegate.getRecordCount(treeName); - } + @Override + public ByteString read(TreeName treeName, ByteSequence key) + { + return delegate.read(treeName, key); + } - @Override - public boolean treeExists(TreeName treeName) - { - return delegate.treeExists(treeName); + @Override + public long getRecordCount(TreeName treeName) + { + return delegate.getRecordCount(treeName); + } + + @Override + public boolean treeExists(TreeName treeName) + { + return delegate.treeExists(treeName); + } } } } From fd50d19dcb85fea732248528f67fc9236f1013cf Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 17 Sep 2026 20:00:15 +0300 Subject: [PATCH 3/3] [#993] Give back what a failed JEStorage open took, and pin the give-back past the database open JEStorage had the shape PDBStorage had before the previous round: the quota taken in buildConfiguration ahead of open0, the double-open guard behind it, a failed `new Environment` leaving the quota and the constructor's listener behind with nobody to close the storage, and a close() which released the quota on every call - three times on the import road, for two acquisitions. It gets the same moves: rejectIfOpen() ahead of buildConfiguration in open() and startImport(), openOrGiveBack() around open0(), the quota released once, an environment without a monitor tolerated. Both storages now give back the quota, the listener and the monitored directory ahead of the database, so that a database whose own close throws keeps nothing else. JEStorageTest, new: the three cases of PDBStorageTest over a directory the server cannot use - a locked directory is not a JE road inside a JVM, DbEnvPool shares the environment. PDBStorageTest gains the case past the database open, with the disk monitor refusing the directory: the volume, the monitor and the quota are given back. FailedBackendOpenTest: a case whose expected failure does not come closes the backend it opened after all, so the case which follows fails on its own assertion rather than on the base DN the previous one left registered. --- .../opends/server/backends/jeb/JEStorage.java | 94 ++++-- .../server/backends/pdb/PDBStorage.java | 37 +-- .../backends/pluggable/RootContainer.java | 2 +- .../server/backends/jeb/JEStorageTest.java | 277 ++++++++++++++++++ .../server/backends/pdb/PDBStorageTest.java | 39 +++ .../pluggable/FailedBackendOpenTest.java | 159 ++++------ 6 files changed, 471 insertions(+), 137 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java index 4db4d9fcaf..919c711207 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java @@ -791,21 +791,7 @@ public void close() trees.clear(); } - if (env != null) - { - DirectoryServer.deregisterMonitorProvider(monitor); - monitor = null; - try - { - env.close(); - env = null; - } - catch (DatabaseException e) - { - throw new IllegalStateException(e); - } - } - + // Given back ahead of the environment, so that an environment whose close fails keeps nothing else. if (memQuota != null) { if (config.getDBCacheSize() > 0) @@ -816,6 +802,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.removeJEChangeListener(this); envConfig = null; @@ -823,6 +813,25 @@ public void close() { diskMonitor.deregisterMonitoredDirectory(getDirectory(), this); } + + if (env != null) + { + // Not yet registered when a failed open got no further than the environment itself. + if (monitor != null) + { + DirectoryServer.deregisterMonitorProvider(monitor); + monitor = null; + } + try + { + env.close(); + env = null; + } + catch (DatabaseException e) + { + throw new IllegalStateException(e); + } + } } @Override @@ -836,8 +845,53 @@ public void open(AccessMode accessMode) throws ConfigException, StorageRuntimeEx // Do not open files on disk return; } + rejectIfOpen(); buildConfiguration(accessMode, false); - open0(); + openOrGiveBack(); + } + + /** + * Refuses to open an environment 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 (env != null) + { + throw new IllegalStateException( + "Database is already open, either the backend is enabled or an import is currently running."); + } + } + + /** + * Opens the environment, 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() throws ConfigException + { + boolean opened = false; + try + { + open0(); + 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) @@ -863,11 +917,6 @@ private void open0() throws ConfigException setupStorageFiles(backendDirectory, config.getDBDirectoryPermissions(), config.dn()); try { - if (env != null) - { - throw new IllegalStateException( - "Database is already open, either the backend is enabled or an import is currently running."); - } env = new Environment(backendDirectory, envConfig); monitor = new JEMonitor(config.getBackendId() + " JE Database", env); DirectoryServer.registerMonitorProvider(monitor); @@ -899,8 +948,9 @@ public T read(final ReadOperation operation) throws Exception @Override public Importer startImport() throws ConfigException, StorageRuntimeException { + rejectIfOpen(); buildConfiguration(AccessMode.READ_WRITE, true); - open0(); + openOrGiveBack(); return new ImporterImpl(); } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java index 5ca9d4744d..bd7c59e184 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java @@ -1102,24 +1102,7 @@ private Configuration buildConfiguration(AccessMode accessMode) @Override public void close() { - if (db != 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(); - db = null; - } - catch (final PersistitException e) - { - throw new IllegalStateException(e); - } - } + // Given back ahead of the database, so that a database whose close fails keeps nothing else. if (memQuota != null) { if (config.getDBCacheSize() > 0) @@ -1140,6 +1123,24 @@ public void close() { diskMonitor.deregisterMonitoredDirectory(getDirectory(), this); } + if (db != 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(); + db = null; + } + catch (final PersistitException e) + { + throw new IllegalStateException(e); + } + } } private static BufferPoolConfiguration getBufferPoolCfg(Configuration dbCfg) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java index 7d6270fa35..e1dbac5b3d 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java @@ -182,7 +182,7 @@ public void run(WriteableTransaction txn) throws Exception * @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 + * {@code PDBStorage}, {@code JEStorage} and {@code JDBCStorage} 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. */ diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java new file mode 100644 index 0000000000..ef1e762f86 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java @@ -0,0 +1,277 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.opends.server.backends.jeb; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; +import static org.forgerock.opendj.config.ConfigurationMock.mockCfg; +import static org.forgerock.opendj.ldap.ByteString.valueOfUtf8; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.File; + +import org.forgerock.opendj.config.server.ConfigException; +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; +import org.forgerock.opendj.server.config.server.JEBackendCfg; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +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.TreeName; +import org.opends.server.backends.pluggable.spi.WriteOperation; +import org.opends.server.backends.pluggable.spi.WriteableTransaction; +import org.opends.server.core.MemoryQuota; +import org.opends.server.core.ServerContext; +import org.opends.server.extensions.DiskSpaceMonitor; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * Tests what a {@link JEStorage} takes as it opens and gives back when the open fails - the twin + * of the same cases on {@code PDBStorageTest}. + */ +@SuppressWarnings("javadoc") +public class JEStorageTest extends DirectoryServerTestCase +{ + private static final String BACKEND_ID = "JEStorageTest"; + /** + * A parent directory under which the storage's own directory is a regular file, so that the open + * fails once its configuration is built - the memory reserved - and before the environment is: + * what a backend whose directory the server cannot use meets. + */ + private static final String BLOCKED_DB_DIRECTORY = BACKEND_ID + "-blocked"; + + private final TreeName treeName = new TreeName("dc=test", "test"); + private ServerContext serverContext; + private JEStorage storage; + + @BeforeClass + public static void startServer() throws Exception + { + TestCaseUtils.startServer(); + } + + @BeforeMethod + public void setUp() throws Exception + { + serverContext = mock(ServerContext.class); + when(serverContext.getMemoryQuota()).thenReturn(new MemoryQuota()); + when(serverContext.getDiskSpaceMonitor()).thenReturn(mock(DiskSpaceMonitor.class)); + + storage = new JEStorage(createBackendCfg(), serverContext); + // the environment is removed on the way in as well as on the way out: a build whose JVM died never ran + // tearDown(), and this class shares a fixed db-directory across methods and across builds + storage.removeStorageFiles(); + storage.open(AccessMode.READ_WRITE); + } + + @AfterMethod + public void tearDown() + { + closeAndRemove(storage); + } + + /** + * Closes the storage and removes its environment, keeping whichever of the two failed first. Removing it + * from a finally would let a removal failure replace the close() failure (JLS 14.20.2) - and a close() that + * throws is exactly the case the removal is here for. + */ + private static void closeAndRemove(JEStorage storage) + { + RuntimeException failure = null; + try + { + storage.close(); + } + catch (RuntimeException e) + { + failure = e; + } + try + { + storage.removeStorageFiles(); + } + catch (RuntimeException e) + { + if (failure == null) + { + failure = e; + } + else + { + failure.addSuppressed(e); + } + } + if (failure != null) + { + throw failure; + } + } + + /** + * 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 + * directory the server cannot use is enabled again and again, each attempt draining one cache + * size. + */ + @Test + public void aStorageWhoseOpenFailedGivesBackWhatItTook() throws Exception + { + final JEBackendCfg cfg = createBackendCfg(); + final JEStorage second = blockedStorage(cfg); + 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 directory which is a file"); + } + catch (ConfigException expected) + { + // What a backend directory the server cannot use does. + } + finally + { + unblock(second); + } + + assertThat(quota.getAvailableMemory()).isEqualTo(availableBefore); + verify(cfg).removeJEChangeListener(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 JEStorage second = blockedStorage(createBackendCfg()); + 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 directory which is a file"); + } + catch (ConfigException expected) + { + // What a backend directory the server cannot use does. + } + finally + { + unblock(second); + } + + 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 environment. + assertThat(read("missing")).isNull(); + } + + /** A storage whose directory is a regular file, which no open of it can use. */ + private JEStorage blockedStorage(JEBackendCfg cfg) throws Exception + { + when(cfg.getDBDirectory()).thenReturn(BLOCKED_DB_DIRECTORY); + final JEStorage blocked = new JEStorage(cfg, serverContext); + final File directory = blocked.getDirectory(); + directory.getParentFile().mkdirs(); + if (!directory.isFile()) + { + assertThat(directory.createNewFile()).as("the file in the way of %s", directory).isTrue(); + } + return blocked; + } + + /** Removes the file in the way of the given storage's directory, and the directory it was made in. */ + private static void unblock(JEStorage blocked) + { + final File directory = blocked.getDirectory(); + directory.delete(); + directory.getParentFile().delete(); + } + + private void createTree() throws Exception + { + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + txn.openTree(treeName, true); + } + }); + } + + private ByteString read(final String key) throws Exception + { + return storage.read(new ReadOperation() + { + @Override + public ByteString run(ReadableTransaction txn) throws Exception + { + return txn.read(treeName, valueOfUtf8(key)); + } + }); + } + + private static JEBackendCfg createBackendCfg() + { + final JEBackendCfg backendCfg = mockCfg(JEBackendCfg.class); + when(backendCfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); + when(backendCfg.getBackendId()).thenReturn(BACKEND_ID); + when(backendCfg.getDBDirectory()).thenReturn(BACKEND_ID); + when(backendCfg.getDBDirectoryPermissions()).thenReturn("755"); + when(backendCfg.getDBCacheSize()).thenReturn(0L); + when(backendCfg.getDBCachePercent()).thenReturn(20); + when(backendCfg.getDBNumCleanerThreads()).thenReturn(2); + when(backendCfg.getDBNumLockTables()).thenReturn(63); + return backendCfg; + } +} diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java index 8780aee0ab..9bcbbb2fe9 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java @@ -22,6 +22,7 @@ import static org.opends.server.util.StaticUtils.*; import static org.forgerock.opendj.ldap.ByteString.*; +import java.io.File; import java.util.concurrent.atomic.AtomicInteger; import org.forgerock.opendj.config.server.ConfigException; @@ -37,6 +38,7 @@ import org.opends.server.backends.pluggable.spi.TreeName; import org.opends.server.backends.pluggable.spi.WriteOperation; import org.opends.server.backends.pluggable.spi.WriteableTransaction; +import org.opends.server.core.DirectoryServer; import org.opends.server.core.MemoryQuota; import org.opends.server.core.ServerContext; import org.opends.server.extensions.DiskSpaceMonitor; @@ -503,6 +505,43 @@ public void openingAnOpenStorageIsRefusedAndTakesNothing() throws Exception assertThat(read("missing")).isNull(); } + /** + * An open which fails once the database is open gives the database back with the rest: the + * volume, or no later open of the backend can take it, and the monitor the open registered. The + * disk monitor is the one thing past the database open that a test can refuse. + */ + @Test + public void aStorageWhoseOpenFailedAfterItsDatabaseOpenedGivesTheDatabaseBack() throws Exception + { + // The volume of setUp() is given up first: held, it fails the open before the database is built. + closeAndRemove(storage); + final DiskSpaceMonitor refusing = mock(DiskSpaceMonitor.class); + doThrow(new IllegalStateException("the directory cannot be monitored")) + .when(refusing).registerMonitoredDirectory(anyString(), any(File.class), anyLong(), anyLong(), any()); + when(serverContext.getDiskSpaceMonitor()).thenReturn(refusing); + final PDBBackendCfg cfg = createBackendCfg(); + 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 when its directory cannot be monitored"); + } + catch (IllegalStateException expected) + { + // What the failure past the database open does. + } + + assertThat(quota.getAvailableMemory()).isEqualTo(availableBefore); + verify(cfg).removePDBChangeListener(second); + assertThat(DirectoryServer.getMonitorProviders()).doesNotContainKey("pdbstoragetest pdb database"); + // The volume was given back: a storage over the same directory opens. + when(serverContext.getDiskSpaceMonitor()).thenReturn(mock(DiskSpaceMonitor.class)); + storage = new PDBStorage(createBackendCfg(), serverContext); + storage.open(AccessMode.READ_WRITE); + } + private void createTree() throws Exception { storage.write(new WriteOperation() diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java index d66fc56d4a..ccc16013e2 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java @@ -22,7 +22,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.opends.server.util.CollectionUtils.newTreeSet; -import static org.testng.Assert.fail; import java.util.ArrayList; import java.util.HashSet; @@ -120,19 +119,8 @@ public void aBackendWhichFailsToOpenLeavesNothingRegistered() throws Exception when(vlvIndexCfg.getFilter()).thenReturn("(&(objectClass=*)"); backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); - try - { - backend.openBackend(); - fail("the backend was expected not to open with a VLV index whose filter does not parse"); - } - catch (InitializationException expected) - { - // What an index the stored configuration names, and the schema no longer supports, does. - } - finally - { - backend.storage.close(); - } + // What an index the stored configuration names, and the schema no longer supports, does. + openExpectingFailure(backend, "the backend was expected not to open with a VLV index whose filter does not parse"); assertThat(stillRegisteredOn(cfg)).isEmpty(); assertThat(stillRegisteredOn(indexCfg)).isEmpty(); @@ -152,19 +140,8 @@ public void anIndexWhichFailsToOpenLeavesNoListenerBehind() throws Exception backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); backend.storage.failOpeningTree("/dc=com,dc=b993/vlv.vlv1"); - try - { - backend.openBackend(); - fail("the backend was expected not to open with a VLV index whose tree cannot be opened"); - } - catch (InitializationException expected) - { - // What a storage which cannot give the index its tree does. - } - finally - { - backend.storage.close(); - } + // What a storage which cannot give the index its tree does. + openExpectingFailure(backend, "the backend was expected not to open with a VLV index whose tree cannot be opened"); assertThat(stillRegisteredOn(cfg)).isEmpty(); assertThat(stillRegisteredOn(vlvIndexCfg)).isEmpty(); @@ -185,28 +162,11 @@ public void aBackendWhichFailsToOpenGivesBackTheStorageItOpened() throws Excepti when(vlvIndexCfg.getFilter()).thenReturn("(&(objectClass=*)"); backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); - final int closesAfterTheFailure; - try - { - try - { - backend.openBackend(); - fail("the backend was expected not to open with a VLV index whose filter does not parse"); - } - catch (InitializationException expected) - { - // What an index the stored configuration names, and the schema no longer supports, does. - } - closesAfterTheFailure = backend.storage.closeCalls(); - } - finally - { - // A no-op once the failed open has given the storage back, and what keeps the tests which - // follow runnable if it has not. - backend.storage.close(); - } + // What an index the stored configuration names, and the schema no longer supports, does. + final int closesByTheFailedOpen = openExpectingFailure(backend, + "the backend was expected not to open with a VLV index whose filter does not parse"); - assertThat(closesAfterTheFailure).isEqualTo(1); + assertThat(closesByTheFailedOpen).isEqualTo(1); } /** @@ -224,17 +184,11 @@ public void aRootContainerWhichCouldNotOpenTheStorageDoesNotCloseIt() throws Exc backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); backend.storage.refuseToOpen(); - try - { - backend.openBackend(); - fail("the backend was expected not to open over a storage whose volume is locked"); - } - catch (InitializationException expected) - { - // What a storage whose volume another process holds does. - } + // What a storage whose volume another process holds does. + final int closesByTheFailedOpen = openExpectingFailure(backend, + "the backend was expected not to open over a storage whose volume is locked"); - assertThat(backend.storage.closeCalls()).isEqualTo(0); + assertThat(closesByTheFailedOpen).isEqualTo(0); assertThat(stillRegisteredOn(cfg)).isEmpty(); } @@ -255,19 +209,9 @@ public void aSecondBaseDNWhichFailsToOpenGivesBackTheFirst() throws Exception backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); backend.storage.failOpeningTree("/dc=com,dc=b993b/id2entry"); - try - { - backend.openBackend(); - fail("the backend was expected not to open with a second base DN whose trees cannot be opened"); - } - catch (InitializationException expected) - { - // What a storage which cannot give the second container its trees does. - } - finally - { - backend.storage.close(); - } + // What a storage which cannot give the second container its trees does. + openExpectingFailure(backend, + "the backend was expected not to open with a second base DN whose trees cannot be opened"); // The road this test is about: the first container had opened before the second one failed. assertThat(backend.storage.openedTrees()).contains("/dc=com,dc=b993/id2entry"); @@ -295,19 +239,8 @@ public void anEntryContainerWhichOpenedButWasNotRegisteredIsGivenBack() throws E // EntryContainer.open() and the one each untrusted index makes as it opens - fails the open // itself, which the container catches. backend.storage.failOpeningCursor("/dc=com,dc=b993/id2entry", () -> anEntryContainerIsRegisteredOn(cfg)); - try - { - backend.openBackend(); - fail("the backend was expected not to open when the highest entry ID cannot be read"); - } - catch (InitializationException expected) - { - // What a storage which cannot position a cursor on the last entry does. - } - finally - { - backend.storage.close(); - } + // What a storage which cannot position a cursor on the last entry does. + openExpectingFailure(backend, "the backend was expected not to open when the highest entry ID cannot be read"); assertThat(stillRegisteredOn(cfg)).isEmpty(); assertThat(stillRegisteredOn(indexCfg)).isEmpty(); @@ -328,7 +261,7 @@ public void anEntryContainerWhichOpenedIsRegisteredOnce() throws Exception final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN)); backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); - backend.openBackend(); + openExpectingSuccess(backend); final List registered; final List registeredOnIndex; final List registeredOnVLVIndex; @@ -369,17 +302,7 @@ public void aReplayedOpenLeavesOneSetOfEntryContainers() throws Exception backend.configureBackend(cfg, serverContext); backend.storage.removeStorageFiles(); backend.storage.conflictAtCommit(1); - try - { - backend.openBackend(); - } - catch (Exception failedToOpen) - { - // Leave the volume closed, or every test which follows fails in openBackend() too and the - // one which actually broke is lost among them. - backend.storage.close(); - throw failedToOpen; - } + openExpectingSuccess(backend); try { assertThat(backend.storage.writeAttempts()).isEqualTo(2); @@ -397,6 +320,50 @@ public void aReplayedOpenLeavesOneSetOfEntryContainers() throws Exception assertThat(stillRegisteredOn(vlvIndexCfg)).isEmpty(); } + /** + * Opens a backend which is not expected to open, and gives back what the attempt left behind: + * the storage a failed open may have left open, or the backend itself when it opened after all - + * its base DNs stay registered with the server otherwise, and every test which follows fails in + * {@code openBackend()} on them rather than on what it is about. + * + * @return how many times the failed open closed the storage, before this method closed it + */ + private static int openExpectingFailure(TrackedBackend backend, String expectation) throws Exception + { + try + { + backend.openBackend(); + } + catch (InitializationException expected) + { + final int closesByTheFailedOpen = backend.storage.closeCalls(); + // A no-op once the failed open has given the storage back, and what keeps the tests which + // follow runnable if it has not. + backend.storage.close(); + return closesByTheFailedOpen; + } + backend.finalizeBackend(); + throw new AssertionError(expectation); + } + + /** + * Opens a backend which is expected to open. One which does not is left with its volume closed, + * or every test which follows fails in {@code openBackend()} too and the one which actually + * broke is lost among them. + */ + private static void openExpectingSuccess(TrackedBackend backend) throws Exception + { + try + { + backend.openBackend(); + } + catch (Exception failedToOpen) + { + backend.storage.close(); + throw failedToOpen; + } + } + /** Every listener added to the backend configuration and not taken off it again. */ private static List stillRegisteredOn(PluggableBackendCfg cfg) throws ConfigException {