From 8077b4a0dc0764c04e847c5bf884bc86c36a4a5c Mon Sep 17 00:00:00 2001
From: asofold <asofold@web.de>
Date: Thu, 13 Apr 2017 12:02:32 +0200
Subject: [PATCH] Extend HashMapLOW interface. Put PlayerData FCFS under lock.

HashMapLOW
* Add a constructor for using an external lock.
* Add putIfAbsent.

DataManager
* Use playerDataMap.putIfAbsent, return the PlayerData instance that has
been there first.
---
 .../utilities/ds/map/HashMapLOW.java          | 54 +++++++++++++++++--
 .../nocheatplus/players/DataManager.java      |  5 +-
 .../nocheatplus/players/PlayerData.java       | 12 ++++-
 3 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/NCPCommons/src/main/java/fr/neatmonster/nocheatplus/utilities/ds/map/HashMapLOW.java b/NCPCommons/src/main/java/fr/neatmonster/nocheatplus/utilities/ds/map/HashMapLOW.java
index de01b84e..3cf864d4 100644
--- a/NCPCommons/src/main/java/fr/neatmonster/nocheatplus/utilities/ds/map/HashMapLOW.java
+++ b/NCPCommons/src/main/java/fr/neatmonster/nocheatplus/utilities/ds/map/HashMapLOW.java
@@ -66,14 +66,17 @@ public class HashMapLOW <K, V> {
             this.value = value;
         }
 
+        @Override
         public K getKey() {
             return key;
         }
 
+        @Override
         public V getValue() {
             return value;
         }
 
+        @Override
         public V setValue(V value) {
             final V oldValue = this.value; 
             this.value = value; 
@@ -140,11 +143,15 @@ public class HashMapLOW <K, V> {
 
         /**
          * Called under lock.
+         * 
          * @param key
          * @param value
+         * @param ifAbsent
+         *            If true, an existing non-null (!) value will not be
+         *            overridden.
          * @return
          */
-        V put(final int hashCode, final K key, final V value) {
+        V put(final int hashCode, final K key, final V value, final boolean ifAbsent) {
             int emptyIndex;
             if (size == 0) {
                 emptyIndex = 0;
@@ -173,7 +180,9 @@ public class HashMapLOW <K, V> {
                 }
                 if (oldEntry != null) {
                     final V oldValue = oldEntry.getValue();
-                    oldEntry.setValue(value);
+                    if (oldValue == null || !ifAbsent) {
+                        oldEntry.setValue(value);
+                    }
                     return oldValue;
                 }
             }
@@ -405,7 +414,7 @@ public class HashMapLOW <K, V> {
     // Instance members
     ///////////////////////
 
-    private final Lock lock = new ReentrantLock();
+    private final Lock lock;
 
     /** Intended/expected size. */
     private final int targetSize;
@@ -420,11 +429,21 @@ public class HashMapLOW <K, V> {
     // TODO: Configurable: allow shrink.
 
     /**
-     * 
+     * Initialize with a ReentrantLock.
      * @param targetSize
      *            Expected (average) number of elements in the map.
      */
     public HashMapLOW(int targetSize) {
+        this(new ReentrantLock(), targetSize);
+    }
+
+    /**
+     * Initialize with a certain lock.
+     * @param lock
+     * @param targetSize
+     */
+    public HashMapLOW(Lock lock, int targetSize) {
+        this.lock = lock;
         this.targetSize = targetSize;
         buckets = newBuckets(targetSize);
     }
@@ -502,6 +521,31 @@ public class HashMapLOW <K, V> {
      * @return
      */
     public V put(final K key, final V value) {
+        return put(key, value, false);
+    }
+
+    /**
+     * Immediate put, only if there is no value or a null value set for the key,
+     * under lock.
+     * 
+     * @param key
+     * @param value
+     * @return
+     */
+    public V putIfAbsent(final K key, final V value) {
+        return put(key, value, true);
+    }
+
+    /**
+     * 
+     * @param key
+     * @param value
+     * @param ifAbsent
+     *            If true, an existing non-null (!) value will not be
+     *            overridden.
+     * @return
+     */
+    private final V put(final K key, final V value, final boolean ifAbsent) {
         final int hashCode = getHashCode(key);
         lock.lock();
         final int index = getBucketIndex(hashCode, buckets.length);
@@ -510,7 +554,7 @@ public class HashMapLOW <K, V> {
             bucket = new LHMBucket<K, V>();
             buckets[index] = bucket;
         }
-        final V oldValue = bucket.put(hashCode, key, value);
+        V oldValue = bucket.put(hashCode, key, value, ifAbsent);
         if (oldValue == null) {
             size ++;
             if (size > (int) (loadFactor * (float) buckets.length)) {
diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/DataManager.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/DataManager.java
index 217a5d19..a732abf8 100644
--- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/DataManager.java
+++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/DataManager.java
@@ -694,9 +694,10 @@ public class DataManager implements Listener, INeedConfig, ComponentRegistry<IRe
             return null;
         }
         else {
+            // Creating this should be mostly harmless.
             final PlayerData newData = new PlayerData(playerId, playerName);
-            instance.playerData.put(playerId, newData);
-            return newData;
+            final PlayerData oldData = instance.playerData.putIfAbsent(playerId, newData);
+            return oldData == null ? newData : oldData;
         }
     }
 
diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/PlayerData.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/PlayerData.java
index c5c14c95..37426e81 100644
--- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/PlayerData.java
+++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/players/PlayerData.java
@@ -25,8 +25,18 @@ import fr.neatmonster.nocheatplus.components.data.IData;
 import fr.neatmonster.nocheatplus.utilities.TickTask;
 
 /**
- * Central player data object.
+ * Central player-specific data object.
+ * <ul>
+ * <li>Access to on-tick request functionality.</li>
+ * <li>TBD: Permission cache.</li>
+ * <li>TBD: Check data.</li>
+ * <li>TBD: Exemptions</li>
+ * <li>...</li>
+ * </ul>
  * <hr>
+ * Creating PlayerData must always be thread-safe and fail-safe.
+ * <hr>
+ * OLD javadocs to be cleaned up (...):<br>
  * On the medium run this is intended to carry all data for the player...
  * <li>Checks data objects.</li>
  * <li>Time stamps for logged out players</li>