From 5d176c691527e3fe4bc529ff8947f3b3648970bd Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Thu, 10 Sep 2026 11:58:57 +0000
Subject: [PATCH] [#916] Keep an update that lands during a ServerState save out of the saved flag (#948)
---
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 107 insertions(+), 11 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java
index 6d2713f..4f2ab78 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java
@@ -13,6 +13,7 @@
*
* Copyright 2006-2010 Sun Microsystems, Inc.
* Portions Copyright 2012-2016 ForgeRock AS.
+ * Portions Copyright 2026 3A Systems, LLC.
*/
package org.opends.server.replication.plugin;
@@ -24,6 +25,8 @@
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedList;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
import org.forgerock.i18n.slf4j.LocalizedLogger;
import org.forgerock.opendj.ldap.ByteString;
@@ -55,6 +58,11 @@
private final DN baseDN;
private final int serverId;
private final ServerState state;
+ /**
+ * Held by the save which is writing the state to the backend. It is taken
+ * with {@link Lock#tryLock()} and never waited for: see {@link #save()}.
+ */
+ private final Lock saveLock = new ReentrantLock();
/**
* The attribute name used to store the state in the backend.
@@ -104,12 +112,70 @@
/**
* Save this object to persistent storage.
+ * <p>
+ * Only one save writes the state at a time: two of them would otherwise each
+ * take their own snapshot, and the write of the older one landing last would
+ * leave a state on disk that is both stale and marked as saved.
+ * <p>
+ * A save which finds another one writing gives up its turn instead of waiting
+ * for it, and this method never blocks. Waiting would close a lock cycle:
+ * when the write goes to the domain configuration entry it ends up in
+ * {@code LDAPReplicationDomain.applyConfigurationChange()}, which takes the
+ * very lock {@code disable()} holds while calling this method.
+ * <p>
+ * Giving up the turn loses nothing, because the state is marked as saved
+ * before the snapshot of the write in flight is taken. Whatever this save
+ * would have written is therefore either already in that snapshot, or has
+ * cleared the flag again after it was set - in which case the flag is still
+ * clear when that write completes, and the next save writes it.
*/
public void save()
{
- if (!state.isSaved())
+ if (state.isSaved())
{
- state.setSaved(updateStateEntry());
+ // Nothing to write: stay out of the way of whoever is writing.
+ return;
+ }
+
+ if (!saveLock.tryLock())
+ {
+ return;
+ }
+ try
+ {
+ if (state.isSaved())
+ {
+ // The save which just completed carried what this one came to write.
+ return;
+ }
+ /*
+ * Mark the state as saved before the snapshot that goes to the backend
+ * is taken, so that an update landing while the write is in flight
+ * clears the flag again and gets written by the next save. Marking it
+ * afterwards would swallow such an update: it is not part of the write
+ * it raced with, yet the state would look saved. The persisted state
+ * would then stay stale until some later update happened to dirty it
+ * again - which, on a domain as quiet as cn=schema, may never happen.
+ */
+ state.setSaved(true);
+ boolean written = false;
+ try
+ {
+ written = updateStateEntry();
+ }
+ finally
+ {
+ if (!written)
+ {
+ // The write reported a failure, or blew up on its way to the
+ // backend: the state is not on disk, so leave it to the next save.
+ state.setSaved(false);
+ }
+ }
+ }
+ finally
+ {
+ saveLock.unlock();
}
}
@@ -118,6 +184,16 @@
*/
public void loadState()
{
+ /*
+ * Whatever the state holds on the way in has, as far as this object knows,
+ * never been written: what follows only merges in what the backend holds.
+ * No shipped path comes in holding anything - the constructor is handed the
+ * state a ReplicationDomain has just created, and loadDataState() empties
+ * it first - so this guards a caller which does not exist yet rather than
+ * one which does.
+ */
+ final boolean hadCSNs = !state.isEmpty();
+
// try to load the state from the base entry.
SearchResultEntry stateEntry = searchBaseEntry();
if (stateEntry == null)
@@ -141,6 +217,11 @@
* Inconsistencies may append after a crash.
*/
checkAndUpdateServerState();
+
+ if (hadCSNs)
+ {
+ state.setSaved(false);
+ }
}
/**
@@ -262,9 +343,8 @@
op.setInternalOperation(true);
op.setSynchronizationOperation(true);
op.setDontSynchronize(true);
- op.run();
- final ResultCode resultCode = op.getResultCode();
+ final ResultCode resultCode = runModify(op);
if (resultCode != ResultCode.SUCCESS
&& !(resultCode == ResultCode.NO_SUCH_OBJECT && serverStateEntryDN.equals(baseDN)))
{
@@ -274,20 +354,36 @@
}
/**
- * Empty the ServerState.
- * After this call the Server State will be in the same state
- * as if it was just created.
+ * Runs the modify operation that writes the state to the backend.
+ * <p>
+ * Kept separate, and overridable, so that a test can reach the point where
+ * the snapshot has been taken but the write has not gone through yet: see
+ * {@code PersistentServerStateTest}. Do not inline it.
+ *
+ * @param op The modify operation carrying the state to be written.
+ * @return A ResultCode indicating if the operation was successful.
+ */
+ ResultCode runModify(ModifyOperationBasis op)
+ {
+ op.run();
+ return op.getResultCode();
+ }
+
+ /**
+ * Empty the ServerState in memory.
+ * <p>
+ * The emptied state is marked as not saved, so the next save writes the empty
+ * state out - which is what {@link #clear()} is after. A caller that only
+ * means to drop the in-memory copy, and expects the backend to keep what it
+ * holds, has to keep saves away until it has loaded the state back.
*/
public void clearInMemory()
{
state.clear();
- state.setSaved(false);
}
/**
- * Empty the ServerState.
- * After this call the Server State will be in the same state
- * as if it was just created.
+ * Empty the ServerState and write the emptied state to persistent storage.
*/
void clear()
{
--
Gitblit v1.10.0