Do not synchronize replication config shutdown
Remove the synchronization of the AutoReloadConfigDecorator
shutdown because of potential deadlocks detected in the replication tests.
When replication events drainage is enabled, the shutdown operation may
deadlock when trying to access the configuration.
Change-Id: Ief6f2bee9124ee0f252e6671aa4a4829c6356b0d
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
index f30efc8..02daa6d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
@@ -31,7 +31,7 @@
public class AutoReloadConfigDecorator implements ReplicationConfig {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private ReplicationFileBasedConfig currentConfig;
+ private volatile ReplicationFileBasedConfig currentConfig;
private long currentConfigTs;
private long lastFailedConfigTs;
@@ -42,6 +42,8 @@
// ReplicationConfig
private final Provider<ReplicationQueue> replicationQueue;
+ private volatile boolean shuttingDown;
+
@Inject
public AutoReloadConfigDecorator(
SitePaths site,
@@ -91,7 +93,8 @@
long lastModified = getLastModified(currentConfig);
try {
if (force
- || (lastModified > currentConfigTs
+ || (!shuttingDown
+ && lastModified > currentConfigTs
&& lastModified > lastFailedConfigTs
&& queue.isRunning()
&& !queue.isReplaying())) {
@@ -134,13 +137,28 @@
return currentConfig.getEventsDirectory();
}
+ /* shutdown() cannot be set as a synchronized method because
+ * it may need to wait for pending events to complete;
+ * e.g. when enabling the drain of replication events before
+ * shutdown.
+ *
+ * As a rule of thumb for synchronized methods, because they
+ * implicitly define a critical section and associated lock,
+ * they should never hold waiting for another resource, otherwise
+ * the risk of deadlock is very high.
+ *
+ * See more background about deadlocks, what they are and how to
+ * prevent them at: https://en.wikipedia.org/wiki/Deadlock
+ */
@Override
- public synchronized int shutdown() {
+ public int shutdown() {
+ this.shuttingDown = true;
return currentConfig.shutdown();
}
@Override
public synchronized void startup(WorkQueue workQueue) {
+ shuttingDown = false;
currentConfig.startup(workQueue);
}
