ARTEMIS-4197 mitigate races w/various config changes

This commit mitigates a number of different potential race conditions
involving configuration changes. Consider the storeAddressSetting method
in o.a.a.a.c.p.i.j.AbstractJournalStorageManager. It mutates
mapPersistedAddressSettings twice - once to delete an entry and once to
put an entry. If another thread executes the recoverAddressSettings
method in between these two operations it will not contain the record
that is being stored. This same race condition exists for a number of
different maps.

This commit mitigates the race condition by ensuring that the relevant
map is mutated only once (i.e. via put).

This commit also refactors a number of methods to avoid duplicated code.

There are no tests associated with this change since there's no
deterministic way to induce the race condition. It relies on static
analysis and existing tests to validate the fix and detect regressions.

Author: jbertram

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/AbstractPersistedAddressSetting.java

@@ -18,10 +18,9 @@
 package org.apache.activemq.artemis.core.persistence.config;
 
 import org.apache.activemq.artemis.api.core.SimpleString;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
 import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
 
-public abstract class AbstractPersistedAddressSetting implements EncodingSupport {
+public abstract class AbstractPersistedAddressSetting extends PersistedConfiguration {
 
    protected long storeId;
 
@@ -38,15 +37,6 @@ public AbstractPersistedAddressSetting(SimpleString addressMatch, AddressSetting
       this.setting = setting;
    }
 
-   public long getStoreId() {
-      return storeId;
-   }
-
-   public AbstractPersistedAddressSetting setStoreId(long storeId) {
-      this.storeId = storeId;
-      return this;
-   }
-
    public SimpleString getAddressMatch() {
       return addressMatch;
    }
@@ -64,4 +54,9 @@ public AbstractPersistedAddressSetting setSetting(AddressSettings setting) {
       this.setting = setting;
       return this;
    }
+
+   @Override
+   public String getName() {
+      return addressMatch.toString();
+   }
 }

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedAddressSetting.java

@@ -18,6 +18,7 @@
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
 import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
 
 /**
@@ -63,4 +64,8 @@ public int getEncodeSize() {
       return addressMatch.sizeof() + setting.getEncodeSize();
    }
 
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.ADDRESS_SETTING_RECORD;
+   }
 }

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedAddressSettingJSON.java

@@ -18,10 +18,10 @@
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
 import org.apache.activemq.artemis.api.core.SimpleString;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
 
-public class PersistedAddressSettingJSON extends AbstractPersistedAddressSetting implements EncodingSupport {
+public class PersistedAddressSettingJSON extends AbstractPersistedAddressSetting {
 
    SimpleString jsonSetting;
 
@@ -68,4 +68,9 @@ public int getEncodeSize() {
    public String toString() {
       return "PersistedAddressSettingJSON{" + "jsonSetting=" + jsonSetting + ", storeId=" + storeId + ", addressMatch=" + addressMatch + ", setting=" + setting + '}';
    }
+
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.ADDRESS_SETTING_RECORD_JSON;
+   }
 }

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedBridgeConfiguration.java

@@ -18,11 +18,10 @@
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
 import org.apache.activemq.artemis.core.config.BridgeConfiguration;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 
-public class PersistedBridgeConfiguration implements EncodingSupport {
+public class PersistedBridgeConfiguration extends PersistedConfiguration {
 
-   private long storeId;
    private BridgeConfiguration bridgeConfiguration;
 
    @Override
@@ -38,14 +37,6 @@ public PersistedBridgeConfiguration() {
       bridgeConfiguration = new BridgeConfiguration();
    }
 
-   public void setStoreId(long id) {
-      this.storeId = id;
-   }
-
-   public long getStoreId() {
-      return storeId;
-   }
-
    @Override
    public int getEncodeSize() {
       return bridgeConfiguration.getEncodeSize();
@@ -61,10 +52,16 @@ public void decode(ActiveMQBuffer buffer) {
       bridgeConfiguration.decode(buffer);
    }
 
+   @Override
    public String getName() {
       return bridgeConfiguration.getParentName();
    }
 
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.BRIDGE_RECORD;
+   }
+
    public BridgeConfiguration getBridgeConfiguration() {
       return bridgeConfiguration;
    }

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedConfiguration.java

@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.persistence.config;
+
+import org.apache.activemq.artemis.core.journal.EncodingSupport;
+
+public abstract class PersistedConfiguration implements EncodingSupport {
+
+   protected long storeId;
+
+   public void setStoreId(long id) {
+      this.storeId = id;
+   }
+
+   public long getStoreId() {
+      return storeId;
+   }
+
+   /**
+    * Returns the name to be used as the key for the map used by the broker to track this configuration implementation.
+    *
+    * @return the name as a String
+    */
+   public abstract String getName();
+
+   public abstract byte getRecordType();
+}

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedConnector.java

@@ -17,12 +17,10 @@
 package org.apache.activemq.artemis.core.persistence.config;
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 import org.apache.activemq.artemis.utils.BufferHelper;
 
-public class PersistedConnector implements EncodingSupport {
-
-   private long storeId;
+public class PersistedConnector extends PersistedConfiguration {
 
    private String url;
 
@@ -36,14 +34,6 @@ public PersistedConnector(String name, String url) {
       this.url = url;
    }
 
-   public void setStoreId(long id) {
-      this.storeId = id;
-   }
-
-   public long getStoreId() {
-      return storeId;
-   }
-
    public void setUrl(String url) {
       this.url = url;
    }
@@ -56,10 +46,16 @@ public void setName(String name) {
       this.name = name;
    }
 
+   @Override
    public String getName() {
       return name;
    }
 
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.CONNECTOR_RECORD;
+   }
+
    @Override
    public int getEncodeSize() {
       int size = 0;

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedDivertConfiguration.java

@@ -18,11 +18,9 @@
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
 import org.apache.activemq.artemis.core.config.DivertConfiguration;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 
-public class PersistedDivertConfiguration implements EncodingSupport {
-
-   private long storeId;
+public class PersistedDivertConfiguration extends PersistedConfiguration {
 
    private DivertConfiguration divertConfiguration;
 
@@ -39,14 +37,6 @@ public PersistedDivertConfiguration() {
       divertConfiguration = new DivertConfiguration();
    }
 
-   public void setStoreId(long id) {
-      this.storeId = id;
-   }
-
-   public long getStoreId() {
-      return storeId;
-   }
-
    @Override
    public int getEncodeSize() {
       return divertConfiguration.getEncodeSize();
@@ -62,10 +52,16 @@ public void decode(ActiveMQBuffer buffer) {
       divertConfiguration.decode(buffer);
    }
 
+   @Override
    public String getName() {
       return divertConfiguration.getName();
    }
 
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.DIVERT_RECORD;
+   }
+
    public DivertConfiguration getDivertConfiguration() {
       return divertConfiguration;
    }

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedKeyValuePair.java

@@ -17,12 +17,10 @@
 package org.apache.activemq.artemis.core.persistence.config;
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 import org.apache.activemq.artemis.utils.BufferHelper;
 
-public class PersistedKeyValuePair implements EncodingSupport {
-
-   private long storeId;
+public class PersistedKeyValuePair extends PersistedConfiguration {
 
    private String mapId;
 
@@ -39,14 +37,6 @@ public PersistedKeyValuePair(String mapId, String key, String value) {
       this.value = value;
    }
 
-   public void setStoreId(long id) {
-      this.storeId = id;
-   }
-
-   public long getStoreId() {
-      return storeId;
-   }
-
    public String getMapId() {
       return mapId;
    }
@@ -59,6 +49,16 @@ public String getValue() {
       return value;
    }
 
+   @Override
+   public String getName() {
+      return mapId;
+   }
+
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.KEY_VALUE_PAIR_RECORD;
+   }
+
    @Override
    public int getEncodeSize() {
       int size = 0;
@@ -85,12 +85,9 @@ public void decode(ActiveMQBuffer buffer) {
    @Override
    public String toString() {
       return "PersistedKeyValuePair [storeId=" + storeId +
-         ", mapId=" +
-         mapId +
-         ", key=" +
-         key +
-         ", value=" +
-         value +
+         ", mapId=" + mapId +
+         ", key=" + key +
+         ", value=" + value +
          "]";
    }
 }

artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedRole.java

@@ -21,13 +21,11 @@
 import java.util.Objects;
 
 import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
-import org.apache.activemq.artemis.core.journal.EncodingSupport;
+import org.apache.activemq.artemis.core.persistence.impl.journal.JournalRecordIds;
 import org.apache.activemq.artemis.utils.BufferHelper;
 import org.apache.activemq.artemis.utils.DataConstants;
 
-public class PersistedRole implements EncodingSupport {
-
-   private long storeId;
+public class PersistedRole extends PersistedConfiguration {
 
    private String username;
 
@@ -41,14 +39,6 @@ public PersistedRole(String username, List<String> roles) {
       this.roles = Objects.requireNonNull(roles);
    }
 
-   public void setStoreId(long id) {
-      this.storeId = id;
-   }
-
-   public long getStoreId() {
-      return storeId;
-   }
-
    public String getUsername() {
       return username;
    }
@@ -105,4 +95,14 @@ public String toString() {
 
       return result.toString();
    }
+
+   @Override
+   public byte getRecordType() {
+      return JournalRecordIds.ROLE_RECORD;
+   }
+
+   @Override
+   public String getName() {
+      return username;
+   }
 }