Conversation
…n 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 OpenIdentityPlatform#993
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The PR closes the leak it names and proves it: four of the five cases are red at BASE exactly as the table says (run here: tests=5 failures=4), and the fifth kills the storageOpened-forced-true mutant.
- The replay give-back at the top of
openAndRegisterEntryContainerscloses theERR_ENTRY_CONTAINER_ALREADY_REGISTEREDroad traced on #883, andaReplayedOpenLeavesOneSetOfEntryContainerspins it both ways: delete the loop → ALREADY_REGISTERED; unregister without close →stillRegisteredOn(cfg)non-empty. catch (Exception e) { close(); throw e; }is pinned byaBackendWhichFailsToOpenLeavesNothingRegisteredthrough the index listener, andEntryContainer.close()has no throwing path (every callee read), so the original exception is the one that escapes.- The issue analysis corrected the report — not an NPE, more than five listeners — before the fix was written.
issue (non-blocking): The entry-container loop in giveUpAfterFailedOpen is pinned by no case — deleting it is green 5/5.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:191-194
Every fixture has one base DN and openEntryContainer runs before registerEntryContainer, so the failing container is never in entryContainers: tests 1-3 iterate an empty map, test 4 fails in storage.open, test 5 succeeds. Measured: the loop removed, FailedBackendOpenTest passes 5/5. The road it exists for — first base DN registered, the second's open fails — is untested. RefusingOneTree matches on getIndexId() alone, so it cannot single out the second container's tree; match on the full name.
// TrackingStorage / RefusingOneTree: refuse by full tree name, not index id alone
if (refused.equals(name.toString())) // "/dc=com,dc=b993b/id2entry"
@Test
public void aSecondBaseDNWhichFailsToOpenGivesBackTheFirst() throws Exception
{
final TrackedBackend backend = new TrackedBackend();
backend.setBackendID(BACKEND_ID);
final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN, DN.valueOf("dc=b993b,dc=com")));
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)
{
}
finally
{
backend.storage.close();
}
assertThat(stillRegisteredOn(cfg)).isEmpty(); // the first container's five
assertThat(stillRegisteredOn(indexCfg)).isEmpty(); // and its cn index
}Pin: with the loop deleted this case must go red on stillRegisteredOn(cfg).
issue (non-blocking): A storage.open() which throws leaves the PDB quota, the PDB change listener and a half-built Persistit behind, and the reason the javadoc gives for not closing it is not the real one.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:137-138, :182-185, :196-199
opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1060, :1085-1095, :1172-1199, :1103-1119
Before open0 can throw, PDBStorage has already registered itself as a PDB change listener (constructor, :1060), acquired dbCacheSize from the MemoryQuota (buildConfiguration, :1090/:1095) and assigned db = new Persistit(dbCfg) (:1182); the catches at :1193-1199 only rewrap. With storageOpened == false nothing gives that back, and RootContainer cannot "always close": PDBStorage.close() with db != null and monitor == null NPEs in DirectoryServer.deregisterMonitorProvider(null) (:1107) before db.close() and the quota release. Every failed enable of a PDB backend whose directory is unwritable, whose volume is corrupt or in use drains one dbCacheSize for the JVM's life and leaves a storage answering PDB config changes — #993 one class down. Pre-existing (BASE closed nothing on any failed-open road); JDBCStorage.open self-unwinds (:1059-1076).
The javadoc's rationale — "a read only root container is opened over the very storage instance the backend holds" — is a road that does not exist: every READ_ONLY open (BackendImpl.exportLDIF:620, verifyBackend:727, rebuildBackend:772, importLDIF:685) is gated on mustOpenRootContainer() i.e. rootContainer == null, so no live root container holds the instance, storage.open() genuinely opens it, and closing it on failure is correct. The false arm fires on exactly one road: storage.open() threw.
// PDBStorage.open: give back what buildConfiguration/open0 took before throwing
@Override
public void open(AccessMode accessMode) throws ConfigException, StorageRuntimeException
{
Reject.ifNull(accessMode, "accessMode must not be null");
if (isBackendIncomplete(accessMode))
{
return;
}
boolean opened = false;
try
{
open0(buildConfiguration(accessMode));
opened = true;
}
finally
{
if (!opened)
{
giveUpFailedOpen(); // db.close() best-effort + db = null; release memQuota; removePDBChangeListener(this)
}
}
}Or: file it as a follow-up against PDBStorage — but in either case rewrite the storageOpened rationale in the giveUpAfterFailedOpen javadoc, the PR body, test 4's javadoc and comment (FailedBackendOpenTest.java:209-211, :229) and the mock message (:474) to the true one: a storage whose open() threw is not one this call can close, and what it took is the storage's own to give back.
issue (non-blocking): A container which opened but fails before registerEntryContainer is reclaimed by nobody.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:288-290
After openEntryContainer returns, the container has registered its five listeners plus one per index; ec.getHighestEntryID(txn) runs before registerEntryContainer(baseDN, ec). A StorageRuntimeException from that cursor read (Persistit.getExchange / Exchange.previous: TreeNotFoundException, PersistitIOException, CorruptVolumeException, TimeoutException) leaves ec a local: giveUpAfterFailedOpen and the replay loop walk entryContainers only, and EntryContainer.open's catch has returned. Not replayed — a fetch never raises RollbackException (both throw sites in Exchange.java are on the store path) — so one leaked set per attempt, and the server continues. Pre-existing window; the PR narrows every other road and leaves this one.
EntryContainer ec = openEntryContainer(baseDN, txn, accessMode);
registerEntryContainer(baseDN, ec); // held before anything else here can throw
EntryID id = ec.getHighestEntryID(txn);suggestion (non-blocking): No case asserts that an opened container is registered — the five registrations can be deleted at their new site and the class stays green.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java:561-566
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java:281-333
Measured: the five registrations moved back to the constructor, everything else at head — 5/5 green, because catch (Exception) { close(); } removes what the constructor added. That revert is unkillable by construction (constructor and open() are always paired), but the delete at the new site is pinnable, and stillRegisteredOn uses List.removeAll, which hides a double registration.
// positive twin, after a successful openBackend()
backend.openBackend();
final List<Object> registered = stillRegisteredOn(cfg);
assertThat(registered).filteredOn(l -> l instanceof EntryContainer).hasSize(1);
// the two private cfg managers, each once as add and once as delete listener: 4, not 0 and not 8
assertThat(registered).filteredOn(l -> l.getClass().getSimpleName().endsWith("IndexCfgManager")).hasSize(4);
// stillRegisteredOn: remove one occurrence per removal, so a double registration is visible
for (Object removed : changeRemoved.getAllValues())
{
registered.remove(removed);
}suggestion (if-minor): aRootContainerWhichCouldNotOpenTheStorageDoesNotCloseIt asserts closeCalls() == 0 only; the root container's own listener removal on the storageOpened == false arm is unpinned.
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java:232
An early return when !storageOpened at the top of giveUpAfterFailedOpen is green 5/5 by reading: the other four cases have storageOpened == true.
assertThat(backend.storage.closeCalls()).isEqualTo(0);
assertThat(stillRegisteredOn(cfg)).isEmpty();…ok, and pin the give-back of the root container Review round 2 of OpenIdentityPlatform#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.
|
All five taken, in 42f6aa2. Each pin was watched to fail on the mutant it is for before the case The loop of
The The container opened but not registered - The positive twin - The listener of the root container on the Green with the change: |
vharseko
added
java
2 participants
Fixes #993.
The defect
EntryContainerregistered itself and its two configuration managers as listeners of the backendconfiguration from its constructor, and only
close()takes them off again.open()caughtStorageRuntimeExceptionalone, while it is declared to throwConfigExceptionand really does -an index type the attribute has no matching rule for, an index protecting both its keys and its
values, and (the most reachable of the three) a VLV filter or sort order which does not parse.
Neither
RootContainer.openEntryContainernoropenAndRegisterEntryContainerscatches it either,so the container is registered nowhere and nothing will ever call its
close().Two things in the report needed correcting, and both are in the analysis on the
issue: the
failure is not a
NullPointerException-id2entryis assigned by the first statement of thetry- and it is more than five listeners, since every index the failed open got throughregistered one of its own. What it costs is worse than an NPE:
ConfigurationHandler.replaceEntryasks every listener on the backend entry whether a change isacceptable and a single
falserejects the whole modify, so an abandoned container can veto achange on the live backend; and past that gate the entry is already stored, so a failure from one
listener turns a change the live container applied into
ERR_CONFIG_FILE_MODIFY_APPLY_FAILED.The change
EntryContaineropen(). A containerwhich did not open is not one a configuration change has anything to be applied to, and nothing
can reach it in between:
open()is called before anything holds it.open()catches every failure rather than the storage ones alone, so aConfigExceptionalsogoes through
close().the end of
open()and a VLV index from its constructor, so the one being opened was not yet oneclose()could find - the hole theStorageRuntimeExceptioncatch already had.RootContaineropen()gives back what it took: the entry containers it registered, its own listener,and the storage - the last unless it was the storage's own
open()that threw. A storage whoseopen failed is not one the root container can close: what that open took before it failed is the
storage's own to give back (below), and there is no other road to that arm - every root container,
read only or not, is opened over a storage no root container holds, since
BackendImplopens oneonly while it has none. Nothing else reclaims any of it:
newRootContainerthrows the instanceaway and
BackendConfigManagerreleases the shared lock without callingcloseBackend()for abackend which never opened, so a volume left open here is one no later attempt to enable that
backend can take.
container which opened has registered every listener it ever will, and only what the registry
holds is given back, so a cursor which fails between the two left one nothing holds.
openAndRegisterEntryContainersruns inside the writeStorage.writemay replay, so it gives upwhat a rolled back attempt registered before opening again. Without it an ordinary write-write
conflict during startup fails the backend with
ERR_ENTRY_CONTAINER_ALREADY_REGISTEREDandleaves that attempt's containers registered - the same shape
BackendImpl.changeBaseDNTreesalready uses for the containers it opens inside a write.
PDBStorageopen()which fails gives back what it took before it failed, asJDBCStorage.openalreadydoes: the cache size
buildConfigurationdrew from the memory quota and the listener theconstructor registered on the backend configuration, and the database itself when the open got
that far. Every failed enable of a PDB backend - its volume held by the storage a previous failed
enable left behind, its directory unwritable - drained one cache size for the life of the JVM and
left a storage answering that backend's configuration changes; and the drained quota is what
every
isConfigurationChangeAcceptableanswers from, so a cache size the server has the memoryfor is refused.
startImport()shares the give-back.give-back never has a live database in front of it.
close()releases the quota once, whatever follows: aclose()after a give-back - or the secondand third
close()importLDIFhas always made on the storage of its root container - releasesnothing more, and it survives a database the failed open registered no monitor for.
The base DN path needs nothing of its own: a container whose
open()throws now closes itself, sothe container
changeBaseDNTreesnever gets to put increatedis reclaimed too.Tests
FailedBackendOpenTest, eight tests, each watched to fail first - against the tree before the changefor the first five, against a mutant of the change for the three which pin what it keeps:
aBackendWhichFailsToOpenLeavesNothingRegisteredanIndexWhichFailsToOpenLeavesNoListenerBehind/dc=com,dc=b993/vlv.vlv1left registeredaBackendWhichFailsToOpenGivesBackTheStorageItOpenedaRootContainerWhichCouldNotOpenTheStorageDoesNotCloseItreturnwhen the storage did not open leaves the root container registeredaReplayedOpenLeavesOneSetOfEntryContainersAn entry container named 'dc=com,dc=b993' is alreadly registeredaSecondBaseDNWhichFailsToOpenGivesBackTheFirstgiveUpAfterFailedOpendeleted: the first base DN's container and its five registrations left behindanEntryContainerWhichOpenedButWasNotRegisteredIsGivenBackanEntryContainerWhichOpenedIsRegisteredOncePDBStorageTest, three tests, each watched to fail first:aStorageWhoseOpenFailedGivesBackWhatItTookclosingAStorageWhoseOpenFailedTakesNothingMoreclose()still released a second timeopeningAnOpenStorageIsRefusedAndTakesNothingRun green with them:
PDBTestCase,EncryptedPDBTestCase,ReplayedConfigChangeTest,OnDiskMergeImporterTest,PersistentCompressedSchemaTest,DN2IDTest,StateTest,ID2EntryTest,ID2ChildrenCountTest,BulkCursorTest,DefaultIndexTest,ImportLDIFTestCase,RebuildIndexTestCase,VerifyIndexTestCase.