Fixed concurrent config modification issues

When multiple threads attempt to add nodes and reorder a config section
concurrency could cause a NPE in ConfigWriter

- Prevented the NPE
- Prevented concurrent modification between multiple node modification methods with a semaphore
This commit is contained in:
Risto Lahtela 2021-04-25 16:02:17 +03:00
parent c7b2050504
commit 202fdbc2e3
2 changed files with 17 additions and 2 deletions

View File

@ -23,6 +23,7 @@
*/ */
package com.djrapitops.plan.settings.config; package com.djrapitops.plan.settings.config;
import com.djrapitops.plan.utilities.UnitSemaphoreAccessLock;
import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.StringUtils;
import java.io.IOException; import java.io.IOException;
@ -40,6 +41,8 @@ import java.util.stream.Collectors;
*/ */
public class ConfigNode { public class ConfigNode {
protected final UnitSemaphoreAccessLock nodeModificationLock = new UnitSemaphoreAccessLock();
protected final String key; protected final String key;
protected ConfigNode parent; protected ConfigNode parent;
@ -128,8 +131,11 @@ public class ConfigNode {
if (parent == null) { if (parent == null) {
throw new IllegalStateException("Can not remove root node from a tree."); throw new IllegalStateException("Can not remove root node from a tree.");
} }
parent.childNodes.remove(key); nodeModificationLock.enter();
parent.nodeOrder.remove(key); parent.nodeOrder.remove(key);
parent.childNodes.remove(key);
nodeModificationLock.exit();
updateParent(null); updateParent(null);
// Remove children recursively to avoid memory leaks // Remove children recursively to avoid memory leaks
@ -149,8 +155,12 @@ public class ConfigNode {
*/ */
protected ConfigNode addChild(ConfigNode child) { protected ConfigNode addChild(ConfigNode child) {
getNode(child.key).ifPresent(ConfigNode::remove); getNode(child.key).ifPresent(ConfigNode::remove);
nodeModificationLock.enter();
childNodes.put(child.key, child); childNodes.put(child.key, child);
nodeOrder.add(child.key); nodeOrder.add(child.key);
nodeModificationLock.exit();
child.updateParent(this); child.updateParent(this);
return child; return child;
} }
@ -197,6 +207,7 @@ public class ConfigNode {
} }
public void reorder(List<String> newOrder) { public void reorder(List<String> newOrder) {
nodeModificationLock.enter();
List<String> oldOrder = nodeOrder; List<String> oldOrder = nodeOrder;
nodeOrder = new ArrayList<>(); nodeOrder = new ArrayList<>();
for (String childKey : newOrder) { for (String childKey : newOrder) {
@ -207,6 +218,7 @@ public class ConfigNode {
// Add those that were not in the new order, but are in the old order. // Add those that were not in the new order, but are in the old order.
oldOrder.removeAll(nodeOrder); oldOrder.removeAll(nodeOrder);
nodeOrder.addAll(oldOrder); nodeOrder.addAll(oldOrder);
nodeModificationLock.exit();
} }
/** /**

View File

@ -95,7 +95,10 @@ public class ConfigWriter {
Map<String, ConfigNode> children = writing.childNodes; Map<String, ConfigNode> children = writing.childNodes;
for (String key : writing.getNodeOrder()) { for (String key : writing.getNodeOrder()) {
ConfigNode node = children.get(key); ConfigNode node = children.get(key);
if (node.value == null && node.nodeOrder.isEmpty()) { // node is null: Inconsistent config node state
// value is null: Has no value (empty)
// nodeOrder is empty: Has no children
if (node == null || node.value == null && node.nodeOrder.isEmpty()) {
continue; continue;
} }