From 9afae366c3818e823bdb98ed5ae63efee9cca50c Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 8 Sep 2026 21:56:21 +0300 Subject: [PATCH 1/2] [#926] Restart the session of a replication domain in one place, under the lock and the generation #959 took the road ExternalChangelogDomain.applyConfigurationChange() reaches the session by - domain.changeConfig(Set, Set) -> restartService() - under serviceStateLock, and had a domain which owns its session refuse the restart. What it left open is the counter: the restart it runs is disableService() plus enableService() with no sessionGeneration bump, so a replay thread which stopped the session and is waiting out its backoff cannot tell that restart from nothing, and the guard in restartSession() still reads isListenerShuttingDown() to stand in for it. serviceStateLock and sessionGeneration move from LDAPReplicationDomain to ReplicationDomain, next to sessionLock, and the counter is bumped by disableService() and enableService() themselves - both final and taken under the lock now - rather than by hand at each place which stops or starts a session, so every restart in the hierarchy is counted and no caller can forget to. A start which throws leaves the generation where it was, so the thread which stopped that session still owns it and may bring it back. The guard in restartSession() is the generation alone: isListenerShuttingDown() stood in for the restarts the counter could not see, and there are none of those left. The starts at domain startup - startPublishService() from the constructor, startListenService() from start() - stay uncounted: no replay thread exists yet to hold a claim on that session. restartService() and readAssuredConfig() take their stop, change and start under the lock as readFractionalConfig() does, so a pair is atomic wherever it is called from rather than only where applyConfigurationChange() holds the lock around it. SessionRestartTest drives the configuration road the issue names against a live replication server, for a domain disabled for a total update and for one which has shut down. Fixes #926. --- .../plugin/LDAPReplicationDomain.java | 66 +------ .../service/ReplicationDomain.java | 187 +++++++++++++----- ...LDAPReplicationDomainConfigChangeTest.java | 4 +- .../plugin/SessionRestartTest.java | 145 ++++++++++++++ 4 files changed, 292 insertions(+), 110 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java index 25b6418f0c..1384cb3655 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java @@ -357,46 +357,6 @@ && getBackend().getBackendID().equals(backend.getBackendID())) { * shortest for as long as the outage lasts. */ private final AtomicInteger consecutiveSessionRestarts = new AtomicInteger(); - /** - * Serialises the session of this domain being stopped and started again: the replay - * thread which restarts it after a failed replay must not race the domain being - * disabled for an import or a restore, or it would bring a broker and a listener - * thread back up on a domain which is supposed to be down. - *

- * Holding it costs something, and knowingly: {@code enableService()} connects to the - * replication servers under this lock, so a shutdown, an import or a configuration - * change which arrives while a replay thread is bringing the session back waits for - * that connect - up to the configured connection timeout when the replication servers - * are unreachable, which is the same outage that failed the replay. Every one of those - * stops the session as its first act, so what they wait for is a session which is about - * to be stopped again. The wait between the stop and the start is deliberately left - * outside the lock, so the waiting is bounded by a connect rather than by the backoff. - *

- * It comes after the configuration backend's update lock and never before it: a write to - * the domain configuration entry holds that lock while it calls - * {@link #applyConfigurationChange(ReplicationDomainCfg)}, which takes this one. So - * nothing may write a configuration entry while holding this lock - that is why neither - * the state {@link #disable()} saves nor the generationId {@link #enable()} stores falls - * back to the domain configuration entry when the base entry of the suffix is missing. - */ - private final Object serviceStateLock = new Object(); - /** - * Bumped every time the session of this domain is stopped or started under - * {@link #serviceStateLock}. A replay thread which stopped the session only starts it - * back if this still is the session it stopped: a configuration change, or the end of - * an import, may have started another one while it was waiting for the backend to - * recover. - *

- * It does not count the sessions {@code changeConfig()} and {@code readAssuredConfig()} - * stop and start, which they do without knowing about it: they run under the lock, so a - * replay thread never observes one of theirs, but a session it stopped may well have - * been replaced by one of theirs while it was waiting. That is why the guard in - * {@link #restartSession(boolean)} reads {@code isListenerShuttingDown()} as well - a - * session started outside this counter leaves it untouched, and only the listener says - * that one is running. - */ - @GuardedBy("serviceStateLock") - private long sessionGeneration; /** * Set by {@link #restartService()} when it left the session of this domain alone, so * that the configuration change which asked for the restart can say so. @@ -896,22 +856,21 @@ private void readFractionalConfig(ReplicationDomainCfg configuration, return; } - // Disable service if configuration changed - final boolean needRestart = needReconnection && allowReconnection; /* * The session is stopped, the configuration it depends on is changed and the session * is started again under the lock which the replay thread restarting the session after * a failed replay holds too: a session brought up in the middle of this would be * reading a fractional configuration which is half way through being changed. The - * pair has to be atomic, which the lock inside disableService()/enableService() does - * not make it. + * stop, the change and the start have to be atomic together, which taking the lock + * inside each of disableService()/enableService() does not make them. */ synchronized (serviceStateLock) { + // Disable service if configuration changed + final boolean needRestart = needReconnection && allowReconnection; if (needRestart) { disableService(); - sessionGeneration++; } else if (needReconnection) { @@ -943,7 +902,6 @@ else if (needReconnection) if (needRestart) { enableService(); - sessionGeneration++; } } } @@ -2513,7 +2471,6 @@ public void shutdown() synchronized (serviceStateLock) { disableService(); - sessionGeneration++; } } @@ -3338,13 +3295,13 @@ private void restartSession(boolean wait) final long stoppedSession; synchronized (serviceStateLock) { - if (shutdown.get() || disabled) + if (ownsItsSession()) { // The domain is going away or is being imported into: it owns its session. return; } disableService(); - stoppedSession = ++sessionGeneration; + stoppedSession = getSessionGeneration(); } if (wait) { @@ -3358,21 +3315,18 @@ private void restartSession(boolean wait) } synchronized (serviceStateLock) { - if (shutdown.get() || disabled - || sessionGeneration != stoppedSession || !isListenerShuttingDown()) + if (ownsItsSession() || getSessionGeneration() != stoppedSession) { /* * The domain went away while this thread was waiting, or the session was stopped * and started again by something else - a configuration change, the end of an * import - in the meantime: the session this thread stopped is gone, so it has - * nothing left to start. The generation says a session was started under this - * lock; the listener says one is running, which is what a restart made outside it - * leaves behind. + * nothing left to start. Every stop and every start of a session is counted, so + * the generation alone tells one session from another. */ return; } enableService(); - sessionGeneration++; } } @@ -4052,7 +4006,6 @@ public void disable() */ disabled = true; disableService(); // This will cut the session and wake up the listener - sessionGeneration++; awaitReplayDrained(); state.save(); state.clearInMemory(); @@ -4220,7 +4173,6 @@ public void enable() try { enableService(); - sessionGeneration++; started = true; } finally diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java index 879bfb498f..9388dc7916 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java @@ -46,6 +46,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import net.jcip.annotations.GuardedBy; import net.jcip.annotations.Immutable; import org.forgerock.i18n.LocalizableMessage; @@ -368,6 +369,44 @@ public ECLIncludes addIncludedAttributes(int serverId, * session of this ReplicationDomain. */ private final Object sessionLock = new Object(); + /** + * Serialises the stopping and the starting of the session of this domain, so that a + * pair of them is atomic: a session stopped so that the configuration it reads can be + * changed must not be brought back in the middle of that change by something else. + *

+ * Holding it costs something, and knowingly: {@link #enableService()} connects to the + * replication servers under this lock, so a shutdown, an import or a configuration + * change which arrives while a replay thread is bringing the session back waits for + * that connect - up to the configured connection timeout when the replication servers + * are unreachable, which is the same outage that failed the replay. Every one of those + * stops the session as its first act, so what they wait for is a session which is + * about to be stopped again. A wait between a stop and a start belongs outside the + * lock, so that the waiting is bounded by a connect rather than by a backoff. + *

+ * It comes after the configuration backend's update lock and never before it: a write + * to the configuration entry of a domain holds that lock while it calls the domain's + * configuration change listener, which takes this one. So nothing may write a + * configuration entry while holding this lock - that is why neither the state a domain + * saves on its way down nor the generationId it stores on its way up falls back to the + * domain configuration entry when the base entry of the suffix is missing. + */ + protected final Object serviceStateLock = new Object(); + /** + * Bumped every time {@link #disableService()} stops the session of this domain or + * {@link #enableService()} starts it, both of them under {@link #serviceStateLock}. It + * is the identity of the session: a thread which stops one and lets the lock go - a + * replay thread waiting out a backoff before it asks for the change it could not apply + * again - only starts it back if this still is the session it stopped, since a + * configuration change or the end of an import may have started another one while it + * was waiting. + *

+ * The starts at domain startup are not counted: {@link #startPublishService()} from + * the constructor of the domain and {@link #startListenService()} from its start bring + * the two halves of the first session up before any replay thread exists to hold a + * claim on it. + */ + @GuardedBy("serviceStateLock") + private long sessionGeneration; /** * The generationId for this replication domain. It is made of a hash of the @@ -3294,34 +3333,38 @@ else if (processUpdate(updateMsg) * It can be useful to disable the Replication Service when the * repository where the replicated information is stored becomes * temporarily unavailable and replicated updates can therefore not - * be replayed during a while. This method is not MT safe. + * be replayed during a while. */ - public void disableService() + public final void disableService() { - synchronized (sessionLock) + synchronized (serviceStateLock) { - /* - * Stop the broker first in order to prevent the listener from reconnecting - see OPENDJ-457. - */ - if (broker != null) - { - broker.stop(); - } - - // Stop the listener thread - if (listenerThread != null) + synchronized (sessionLock) { - listenerThread.initiateShutdown(); - try + /* + * Stop the broker first in order to prevent the listener from reconnecting - see OPENDJ-457. + */ + if (broker != null) { - listenerThread.join(); + broker.stop(); } - catch (InterruptedException e) + + // Stop the listener thread + if (listenerThread != null) { - // Give up waiting. + listenerThread.initiateShutdown(); + try + { + listenerThread.join(); + } + catch (InterruptedException e) + { + // Give up waiting. + } + listenerThread = null; } - listenerThread = null; } + sessionGeneration++; } } @@ -3338,6 +3381,22 @@ protected final boolean isListenerShuttingDown() return tmp == null || tmp.isShutdownInitiated(); } + /** + * Returns the generation of the session of this domain, which changes every time the + * session is stopped or started. + *

+ * It only says anything while {@link #serviceStateLock} is held, and is meant to be + * read under the lock which stopped a session and read again under the lock which + * starts it back: the session stopped is gone when the two differ. + * + * @return the generation of the session of this domain + */ + @GuardedBy("serviceStateLock") + protected final long getSessionGeneration() + { + return sessionGeneration; + } + /** * Restart the Replication service after a {@link #disableService()}. *

@@ -3348,14 +3407,23 @@ protected final boolean isListenerShuttingDown() * If some data have changed in the repository during the period of time when * the Replication Service was disabled, this {@link ServerState} should * therefore be updated by the Replication Domain subclass before calling this - * method. This method is not MT safe. + * method. */ - public void enableService() + public final void enableService() { - synchronized (sessionLock) + synchronized (serviceStateLock) { - broker.start(); - startListenService(); + synchronized (sessionLock) + { + broker.start(); + startListenService(); + } + /* + * Counted once the session really is up: a start which threw leaves the generation + * where it was, so the thread which stopped this session still owns it and may try + * to bring it back. + */ + sessionGeneration++; } } @@ -3397,14 +3465,20 @@ public void changeConfig(Set includeAttributes, * Stops the session of this domain and starts it again, so that it comes up on the * configuration which has just changed. *

- * A subclass may leave it alone: a domain which is shutting down, or which was disabled - * for a total update, owns its session and is not given one back by a configuration - * change. One which does reports it through {@link #onSessionRestartSuppressed()}. + * The pair is taken under {@link #serviceStateLock}, so that nothing starts a session + * back between the stop and the start, and both halves are counted by the session + * generation. A subclass may leave it alone: a domain which is shutting + * down, or which was disabled for a total update, owns its session and is not given one + * back by a configuration change. One which does reports it through + * {@link #onSessionRestartSuppressed()}. */ protected void restartService() { - disableService(); - enableService(); + synchronized (serviceStateLock) + { + disableService(); + enableService(); + } } /** @@ -3874,32 +3948,41 @@ public CSN getLastLocalChange() protected void readAssuredConfig(ReplicationDomainCfg config, boolean allowReconnection) { - // Disconnect if required: changing configuration values before - // disconnection would make assured replication used immediately and - // disconnection could cause some timeouts error. - final boolean needReconnection = needReconnection(config); - final boolean needRestart = needReconnection && allowReconnection; - if (needRestart) - { - disableService(); - } - else if (needReconnection) - { - onSessionRestartSuppressed(); - } /* - * Stored whether or not the session was restarted for it, as the fractional - * configuration is: the assured timeout is the one property a session does not have to - * be restarted for, so a change carrying it alone - reported as applied and then - * dropped, before - is applied here. A caller which does not allow the reconnection - * has no session running assured replication either: the domain is being built, is - * shutting down, or is disabled for the length of a total update, and the session its - * enable() starts reads what is stored here. + * The stop, the change and the start are taken together under serviceStateLock: a + * session brought up in between, by a replay thread restarting the session after a + * failed replay, would negotiate an assured configuration which is half way through + * being changed. */ - assuredConfig = config; - if (needRestart) + synchronized (serviceStateLock) { - enableService(); + // Disconnect if required: changing configuration values before + // disconnection would make assured replication used immediately and + // disconnection could cause some timeouts error. + final boolean needReconnection = needReconnection(config); + final boolean needRestart = needReconnection && allowReconnection; + if (needRestart) + { + disableService(); + } + else if (needReconnection) + { + onSessionRestartSuppressed(); + } + /* + * Stored whether or not the session was restarted for it, as the fractional + * configuration is: the assured timeout is the one property a session does not have + * to be restarted for, so a change carrying it alone - reported as applied and then + * dropped, before - is applied here. A caller which does not allow the reconnection + * has no session running assured replication either: the domain is being built, is + * shutting down, or is disabled for the length of a total update, and the session + * its enable() starts reads what is stored here. + */ + assuredConfig = config; + if (needRestart) + { + enableService(); + } } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/LDAPReplicationDomainConfigChangeTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/LDAPReplicationDomainConfigChangeTest.java index 3d68bf475b..50dd0177a7 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/LDAPReplicationDomainConfigChangeTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/LDAPReplicationDomainConfigChangeTest.java @@ -45,6 +45,7 @@ import org.opends.server.core.ModifyOperation; import org.opends.server.replication.ReplicationTestCase; import org.opends.server.replication.common.AssuredMode; +import org.opends.server.replication.service.ReplicationDomain; import org.testng.annotations.Test; /** @@ -541,7 +542,8 @@ private static boolean waitForBlockedOn(Thread thread, Object monitor) throws Ex private static Object serviceStateLockOf(LDAPReplicationDomain domain) throws Exception { - final Field serviceStateLock = LDAPReplicationDomain.class.getDeclaredField("serviceStateLock"); + // Declared where the session lives, next to disableService()/enableService() + final Field serviceStateLock = ReplicationDomain.class.getDeclaredField("serviceStateLock"); serviceStateLock.setAccessible(true); return serviceStateLock.get(domain); } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java new file mode 100644 index 0000000000..8acdd66f6e --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java @@ -0,0 +1,145 @@ +/* + * 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.replication.plugin; + +import static org.assertj.core.api.Assertions.*; +import static org.opends.server.TestCaseUtils.*; +import static org.testng.Assert.*; + +import java.util.SortedSet; +import java.util.TreeSet; + +import org.forgerock.opendj.ldap.DN; +import org.forgerock.opendj.ldap.ResultCode; +import org.opends.server.TestCaseUtils; +import org.opends.server.replication.ReplicationTestCase; +import org.opends.server.replication.server.ReplServerFakeConfiguration; +import org.opends.server.replication.server.ReplicationServer; +import org.testng.annotations.Test; + +/** + * Tests that a configuration change does not start the session of a domain which stopped + * its own session: a domain which is shutting down, or whose data is being replaced, owns + * its session and is the one which brings it back. + */ +@SuppressWarnings("javadoc") +public class SessionRestartTest extends ReplicationTestCase +{ + private static final int RS_ID = 601; + private static final int DS_ID = 1; + private static final int GROUP_ID = 1; + + @Test + public void aConfigurationChangeDoesNotStartTheSessionOfADisabledDomain() throws Exception + { + final DN baseDN = DN.valueOf(TEST_ROOT_DN_STRING); + ReplicationServer replicationServer = null; + LDAPReplicationDomain domain = null; + try + { + final int rsPort = TestCaseUtils.findFreePort(); + replicationServer = createReplicationServer(rsPort, "sessionRestartTestDisabledDb"); + + final DomainFakeCfg domainCfg = newDomainCfg(baseDN, rsPort); + domain = MultimasterReplication.createNewDomain(domainCfg); + domain.start(); + assertTrue(domain.isConnected()); + + // The data this domain replicates is about to be replaced by an import or a restore. + domain.disable(); + assertFalse(domain.isConnected()); + + changeEclIncludes(domain, domainCfg); + + assertFalse(domain.isConnected(), + "a configuration change started the session of a disabled domain"); + } + finally + { + if (domain != null) + { + MultimasterReplication.deleteDomain(baseDN); + } + remove(replicationServer); + } + } + + @Test + public void aConfigurationChangeDoesNotStartTheSessionOfAShutDownDomain() throws Exception + { + final DN baseDN = DN.valueOf(TEST_ROOT_DN_STRING); + ReplicationServer replicationServer = null; + LDAPReplicationDomain domain = null; + try + { + final int rsPort = TestCaseUtils.findFreePort(); + replicationServer = createReplicationServer(rsPort, "sessionRestartTestShutdownDb"); + + final DomainFakeCfg domainCfg = newDomainCfg(baseDN, rsPort); + domain = MultimasterReplication.createNewDomain(domainCfg); + domain.start(); + assertTrue(domain.isConnected()); + + domain.shutdown(); + assertFalse(domain.isConnected()); + + changeEclIncludes(domain, domainCfg); + + assertFalse(domain.isConnected(), + "a configuration change started the session of a domain which has shut down"); + } + finally + { + if (domain != null) + { + MultimasterReplication.deleteDomain(baseDN); + } + remove(replicationServer); + } + } + + /** + * Applies a configuration change which changes the attributes the external changelog + * includes. The domain hands it to {@code ExternalChangelogDomain}, which asks for the + * session to be restarted so that the replication server hears the new list. + */ + private void changeEclIncludes(LDAPReplicationDomain domain, DomainFakeCfg domainCfg) + throws Exception + { + final SortedSet eclIncludes = new TreeSet<>(); + eclIncludes.add("cn"); + domainCfg.setExternalChangelogDomain( + new ExternalChangelogDomainFakeCfg(true, eclIncludes, new TreeSet())); + + assertEquals(domain.applyConfigurationChange(domainCfg).getResultCode(), ResultCode.SUCCESS); + // the change did reach the external changelog configuration of the domain + assertThat(domain.getEclIncludes()).contains("cn"); + } + + private DomainFakeCfg newDomainCfg(DN baseDN, int rsPort) + { + final SortedSet replServers = new TreeSet<>(); + replServers.add("localhost:" + rsPort); + return new DomainFakeCfg(baseDN, DS_ID, replServers, GROUP_ID); + } + + private ReplicationServer createReplicationServer(int rsPort, String dbDir) throws Exception + { + final ReplServerFakeConfiguration conf = new ReplServerFakeConfiguration( + rsPort, dbDir, 0, RS_ID, 0, 100, new TreeSet(), GROUP_ID, 1000, 5000); + return new ReplicationServer(conf); + } +} From 1ee01f5bfd7740c4a146e1bfb15ab42140332a68 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 14 Sep 2026 10:42:07 +0300 Subject: [PATCH 2/2] [#926] Take both changeConfig() roads under serviceStateLock, and assert the refused restart is reported Review round 2 of #974. changeConfig(ReplicationDomainCfg) and changeConfig(Set, Set) in ReplicationDomain ran their change - the broker properties, the attributes the external changelog publishes - outside serviceStateLock, and only the restartService() at the end of each took it: atomic where applyConfigurationChange() and the LDAPReplicationDomain override held the lock around them, and nowhere else. Both take the lock around the change and the restart now, the shape restartService() and readAssuredConfig() have. Neither broker.changeConfig() nor setEclIncludes() takes a lock of its own, so the order stays serviceStateLock -> sessionLock -> startStopLock. The override which used to supply the lock is gone; externalChangelogConfigurationChangesTheSessionUnderTheServiceStateLock pins the base. The getSessionGeneration() javadoc said the generation changes every time the session is stopped or started, which the javadoc of the field contradicts: the starts at domain startup are not counted. SessionRestartTest asserts on both roads that the refused restart is reported as adminActionRequired rather than as fully applied. --- .../plugin/LDAPReplicationDomain.java | 17 ---------- .../service/ReplicationDomain.java | 34 ++++++++++++++----- .../plugin/SessionRestartTest.java | 6 +++- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java index 1384cb3655..a62c3f60f7 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java @@ -4971,23 +4971,6 @@ private LocalizableMessage configChangeFailed(ReplicationDomainCfg domCfg, Excep domCfg.getBaseDN(), stackTraceToSingleLineString(e)); } - /** - * {@inheritDoc} - *

- * Taken under {@link #serviceStateLock} like every other configuration change: this one - * comes from the external changelog domain - from the entry of its own, or from - * {@link #applyECLConfiguration} - and it restarts the session as well. - */ - @Override - public void changeConfig(Set includeAttributes, - Set includeAttributesForDeletes) - { - synchronized (serviceStateLock) - { - super.changeConfig(includeAttributes, includeAttributesForDeletes); - } - } - @Override public boolean isConfigurationChangeAcceptable( ReplicationDomainCfg configuration, List unacceptableReasons) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java index 9388dc7916..84cf110a1d 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java @@ -3382,8 +3382,10 @@ protected final boolean isListenerShuttingDown() } /** - * Returns the generation of the session of this domain, which changes every time the - * session is stopped or started. + * Returns the generation of the session of this domain: bumped by + * {@link #disableService()} and {@link #enableService()} under + * {@link #serviceStateLock}, not by the starts at domain startup (see + * {@code sessionGeneration}). *

* It only says anything while {@link #serviceStateLock} is held, and is meant to be * read under the lock which stopped a session and read again under the lock which @@ -3429,21 +3431,34 @@ public final void enableService() /** * Change some ReplicationDomain parameters. + *

+ * The change and the restart it may call for are taken together under + * {@link #serviceStateLock}, as {@link #readAssuredConfig(ReplicationDomainCfg, boolean)} + * takes its own: a session brought up between the two would negotiate the broker + * properties which are half way through being changed. * * @param config * The new configuration that this domain should now use. */ protected void changeConfig(ReplicationDomainCfg config) { - if (broker != null && broker.changeConfig(config)) + synchronized (serviceStateLock) { - restartService(); + if (broker != null && broker.changeConfig(config)) + { + restartService(); + } } } /** * Applies a configuration change to the attributes which should be included * in the ECL. + *

+ * Taken under {@link #serviceStateLock} like every other configuration change: this one + * comes from the external changelog domain - from the entry of its own, or from the + * domain configuration change which reads that entry - and it restarts the session as + * well, so the attributes and the restart go together. * * @param includeAttributes * attributes to be included with all change records. @@ -3453,11 +3468,14 @@ protected void changeConfig(ReplicationDomainCfg config) public void changeConfig(Set includeAttributes, Set includeAttributesForDeletes) { - final boolean attrsModified = setEclIncludes( - getServerId(), includeAttributes, includeAttributesForDeletes); - if (attrsModified && broker != null) + synchronized (serviceStateLock) { - restartService(); + final boolean attrsModified = setEclIncludes( + getServerId(), includeAttributes, includeAttributesForDeletes); + if (attrsModified && broker != null) + { + restartService(); + } } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java index 8acdd66f6e..04a9e5ab95 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java @@ -22,6 +22,7 @@ import java.util.SortedSet; import java.util.TreeSet; +import org.forgerock.opendj.config.server.ConfigChangeResult; import org.forgerock.opendj.ldap.DN; import org.forgerock.opendj.ldap.ResultCode; import org.opends.server.TestCaseUtils; @@ -124,7 +125,10 @@ private void changeEclIncludes(LDAPReplicationDomain domain, DomainFakeCfg domai domainCfg.setExternalChangelogDomain( new ExternalChangelogDomainFakeCfg(true, eclIncludes, new TreeSet())); - assertEquals(domain.applyConfigurationChange(domainCfg).getResultCode(), ResultCode.SUCCESS); + final ConfigChangeResult ccr = domain.applyConfigurationChange(domainCfg); + assertEquals(ccr.getResultCode(), ResultCode.SUCCESS, ccr.getMessages().toString()); + // the restart the change asked for was refused, and said so rather than reported as applied + assertTrue(ccr.adminActionRequired(), "the refused restart was reported as fully applied"); // the change did reach the external changelog configuration of the domain assertThat(domain.getEclIncludes()).contains("cn"); }