#949 Create ExpiringSet, integrate into SessionManager

src/main/java/fr/xephi/authme/data/SessionManager.java

@@ -5,13 +5,10 @@ import fr.xephi.authme.initialization.HasCleanup;
 import fr.xephi.authme.initialization.SettingsDependent;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.PluginSettings;
+import fr.xephi.authme.util.ExpiringSet;
 
 import javax.inject.Inject;
-import java.util.Iterator;
+import java.util.concurrent.TimeUnit;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
-import static fr.xephi.authme.util.Utils.MILLIS_PER_MINUTE;
 
 /**
  * Manages sessions, allowing players to be automatically logged in if they join again
@@ -19,15 +16,14 @@ import static fr.xephi.authme.util.Utils.MILLIS_PER_MINUTE;
  */
 public class SessionManager implements SettingsDependent, HasCleanup {
 
-    // Player -> expiration of session in milliseconds
+    private final ExpiringSet<String> sessions;
-    private final Map<String, Long> sessions = new ConcurrentHashMap<>();
-
     private boolean enabled;
-    private int timeoutInMinutes;
 
     @Inject
     SessionManager(Settings settings) {
-        reload(settings);
+        long timeout = settings.getProperty(PluginSettings.SESSIONS_TIMEOUT);
+        sessions = new ExpiringSet<>(timeout, TimeUnit.MINUTES);
+        enabled = timeout > 0 && settings.getProperty(PluginSettings.SESSIONS_ENABLED);
     }
 
     /**
@@ -37,13 +33,7 @@ public class SessionManager implements SettingsDependent, HasCleanup {
      * @return True if a session is found.
      */
     public boolean hasSession(String name) {
-        if (enabled) {
+        return enabled && sessions.contains(name.toLowerCase());
-            Long timeout = sessions.get(name.toLowerCase());
-            if (timeout != null) {
-                return System.currentTimeMillis() <= timeout;
-            }
-        }
-        return false;
     }
 
     /**
@@ -53,8 +43,7 @@ public class SessionManager implements SettingsDependent, HasCleanup {
      */
     public void addSession(String name) {
         if (enabled) {
-            long timeout = System.currentTimeMillis() + timeoutInMinutes * MILLIS_PER_MINUTE;
+            sessions.add(name.toLowerCase());
-            sessions.put(name.toLowerCase(), timeout);
         }
     }
 
@@ -64,12 +53,13 @@ public class SessionManager implements SettingsDependent, HasCleanup {
      * @param name The name of the player.
      */
     public void removeSession(String name) {
-        this.sessions.remove(name.toLowerCase());
+        sessions.remove(name.toLowerCase());
     }
 
     @Override
     public void reload(Settings settings) {
-        timeoutInMinutes = settings.getProperty(PluginSettings.SESSIONS_TIMEOUT);
+        long timeoutInMinutes = settings.getProperty(PluginSettings.SESSIONS_TIMEOUT);
+        sessions.setExpiration(timeoutInMinutes, TimeUnit.MINUTES);
         boolean oldEnabled = enabled;
         enabled = timeoutInMinutes > 0 && settings.getProperty(PluginSettings.SESSIONS_ENABLED);
 
@@ -82,16 +72,8 @@ public class SessionManager implements SettingsDependent, HasCleanup {
 
     @Override
     public void performCleanup() {
-        if (!enabled) {
+        if (enabled) {
-            return;
+            sessions.removeExpiredEntries();
-        }
-        final long currentTime = System.currentTimeMillis();
-        Iterator<Map.Entry<String, Long>> iterator = sessions.entrySet().iterator();
-        while (iterator.hasNext()) {
-            Map.Entry<String, Long> entry = iterator.next();
-            if (entry.getValue() < currentTime) {
-                iterator.remove();
-            }
         }
     }
 }

src/main/java/fr/xephi/authme/util/ExpiringSet.java

@@ -0,0 +1,97 @@
+package fr.xephi.authme.util;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Set whose entries expire after a configurable amount of time. Once an entry
+ * has expired, the set will act as if the entry no longer exists. Time starts
+ * counting after the entry has been inserted.
+ * <p>
+ * Internally, expired entries are not cleared automatically. A cleanup can be
+ * triggered with {@link #removeExpiredEntries()}. Adding an entry that is
+ * already present effectively resets its expiration.
+ *
+ * @param <E> the type of the entries
+ */
+public class ExpiringSet<E> {
+
+    private Map<E, Long> entries = new ConcurrentHashMap<>();
+    private long expirationMillis;
+
+    /**
+     * Constructor.
+     *
+     * @param duration the duration of time after which entries expire
+     * @param unit the time unit in which {@code duration} is expressed
+     */
+    public ExpiringSet(long duration, TimeUnit unit) {
+        setExpiration(duration, unit);
+    }
+
+    /**
+     * Adds an entry to the set.
+     *
+     * @param entry the entry to add
+     */
+    public void add(E entry) {
+        entries.put(entry, System.currentTimeMillis() + expirationMillis);
+    }
+
+    /**
+     * Returns whether this set contains the given entry, if it hasn't expired.
+     *
+     * @param entry the entry to check
+     * @return true if the entry is present and not expired, false otherwise
+     */
+    public boolean contains(E entry) {
+        Long expiration = entries.get(entry);
+        return expiration != null && expiration > System.currentTimeMillis();
+    }
+
+    /**
+     * Removes the given entry from the set (if present).
+     *
+     * @param entry the entry to remove
+     */
+    public void remove(E entry) {
+        entries.remove(entry);
+    }
+
+    /**
+     * Removes all entries from the set.
+     */
+    public void clear() {
+        entries.clear();
+    }
+
+    /**
+     * Removes all entries which have expired from the internal structure.
+     */
+    public void removeExpiredEntries() {
+        entries.entrySet().removeIf(entry -> System.currentTimeMillis() > entry.getValue());
+    }
+
+    /**
+     * Sets a new expiration duration. Note that already present entries
+     * will still make use of the old expiration.
+     *
+     * @param duration the duration of time after which entries expire
+     * @param unit the time unit in which {@code duration} is expressed
+     */
+    public void setExpiration(long duration, TimeUnit unit) {
+        this.expirationMillis = unit.toMillis(duration);
+    }
+
+    /**
+     * Returns whether this map is empty. This reflects the state of the
+     * internal map, which may contain expired entries only. The result
+     * may change after running {@link #removeExpiredEntries()}.
+     *
+     * @return true if map is really empty, false otherwise
+     */
+    public boolean isEmpty() {
+        return entries.isEmpty();
+    }
+}

src/test/java/fr/xephi/authme/data/SessionManagerTest.java

@@ -3,19 +3,17 @@ package fr.xephi.authme.data;
 import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.PluginSettings;
+import fr.xephi.authme.util.ExpiringSet;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.junit.MockitoJUnitRunner;
 
-import java.util.Map;
-
-import static org.hamcrest.Matchers.aMapWithSize;
-import static org.hamcrest.Matchers.anEmptyMap;
-import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 
 /**
  * Test for {@link SessionManager}.
@@ -91,24 +89,6 @@ public class SessionManagerTest {
         assertThat(manager.hasSession(player), equalTo(false));
     }
 
-    @Test
-    public void shouldDenySessionIfTimeoutHasExpired() {
-        // given
-        int timeout = 20;
-        Settings settings = mockSettings(true, timeout);
-        String player = "patrick";
-        SessionManager manager = new SessionManager(settings);
-        Map<String, Long> sessions = getSessionsMap(manager);
-        // Add session entry for player that just has expired
-        sessions.put(player, System.currentTimeMillis() - 1000);
-
-        // when
-        boolean result = manager.hasSession(player);
-
-        // then
-        assertThat(result, equalTo(false));
-    }
-
     @Test
     public void shouldClearAllSessionsAfterDisable() {
         // given
@@ -121,7 +101,7 @@ public class SessionManagerTest {
         manager.reload(mockSettings(false, 20));
 
         // then
-        assertThat(getSessionsMap(manager), anEmptyMap());
+        assertThat(getSessionsMap(manager).isEmpty(), equalTo(true));
     }
 
     @Test
@@ -129,18 +109,14 @@ public class SessionManagerTest {
         // given
         Settings settings = mockSettings(true, 1);
         SessionManager manager = new SessionManager(settings);
-        Map<String, Long> sessions = getSessionsMap(manager);
+        ExpiringSet<String> expiringSet = mockExpiringSet();
-        sessions.put("somebody", System.currentTimeMillis() - 123L);
+        setSessionsMap(manager, expiringSet);
-        sessions.put("someone", System.currentTimeMillis() + 4040L);
-        sessions.put("anyone", System.currentTimeMillis() - 1000L);
-        sessions.put("everyone", System.currentTimeMillis() + 60000L);
 
         // when
         manager.performCleanup();
 
         // then
-        assertThat(sessions, aMapWithSize(2));
+        verify(expiringSet).removeExpiredEntries();
-        assertThat(sessions.keySet(), containsInAnyOrder("someone", "everyone"));
     }
 
     @Test
@@ -148,23 +124,28 @@ public class SessionManagerTest {
         // given
         Settings settings = mockSettings(false, 1);
         SessionManager manager = new SessionManager(settings);
-        Map<String, Long> sessions = getSessionsMap(manager);
+        ExpiringSet<String> expiringSet = mockExpiringSet();
-        sessions.put("somebody", System.currentTimeMillis() - 123L);
+        setSessionsMap(manager, expiringSet);
-        sessions.put("someone", System.currentTimeMillis() + 4040L);
-        sessions.put("anyone", System.currentTimeMillis() - 1000L);
-        sessions.put("everyone", System.currentTimeMillis() + 60000L);
 
         // when
         manager.performCleanup();
 
         // then
-        assertThat(sessions, aMapWithSize(4)); // map not changed -> no cleanup performed
+        verify(expiringSet, never()).removeExpiredEntries();
     }
 
-    private static Map<String, Long> getSessionsMap(SessionManager manager) {
+    private static ExpiringSet<String> getSessionsMap(SessionManager manager) {
         return ReflectionTestUtils.getFieldValue(SessionManager.class, manager, "sessions");
     }
 
+    private static void setSessionsMap(SessionManager manager, ExpiringSet<String> sessionsMap) {
+        ReflectionTestUtils.setField(SessionManager.class, manager, "sessions", sessionsMap);
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> ExpiringSet<T> mockExpiringSet() {
+        return mock(ExpiringSet.class);
+    }
 
     private static Settings mockSettings(boolean isEnabled, int sessionTimeout) {
         Settings settings = mock(Settings.class);

src/test/java/fr/xephi/authme/util/ExpiringSetTest.java

@@ -0,0 +1,91 @@
+package fr.xephi.authme.util;
+
+import org.junit.Test;
+
+import java.util.concurrent.TimeUnit;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Test for {@link ExpiringSet}.
+ */
+public class ExpiringSetTest {
+
+    @Test
+    public void shouldAddEntry() {
+        // given
+        ExpiringSet<String> set = new ExpiringSet<>(10, TimeUnit.MINUTES);
+
+        // when
+        set.add("authme");
+
+        // then
+        assertThat(set.contains("authme"), equalTo(true));
+        assertThat(set.contains("other"), equalTo(false));
+    }
+
+    @Test
+    public void shouldRemoveEntries() {
+        // given
+        ExpiringSet<Integer> set = new ExpiringSet<>(20, TimeUnit.SECONDS);
+        set.add(20);
+        set.add(40);
+
+        // when
+        set.remove(40);
+        set.remove(60);
+
+        // then
+        assertThat(set.contains(20), equalTo(true));
+        assertThat(set.contains(40), equalTo(false));
+        assertThat(set.contains(60), equalTo(false));
+    }
+
+    @Test
+    public void shouldHandleNewExpirationAndSupportNegativeValues() {
+        // given
+        ExpiringSet<Character> set = new ExpiringSet<>(800, TimeUnit.MILLISECONDS);
+        set.add('A');
+
+        // when
+        set.setExpiration(-10, TimeUnit.SECONDS);
+        set.add('Y');
+
+        // then
+        assertThat(set.contains('A'), equalTo(true));
+        assertThat(set.contains('Y'), equalTo(false));
+    }
+
+    @Test
+    public void shouldClearAllValues() {
+        // given
+        ExpiringSet<String> set = new ExpiringSet<>(1, TimeUnit.MINUTES);
+        set.add("test");
+
+        // when / then
+        assertThat(set.isEmpty(), equalTo(false));
+        set.clear();
+        assertThat(set.isEmpty(), equalTo(true));
+        assertThat(set.contains("test"), equalTo(false));
+    }
+
+    @Test
+    public void shouldClearExpiredValues() {
+        // given
+        ExpiringSet<Integer> set = new ExpiringSet<>(2, TimeUnit.HOURS);
+        set.add(2);
+        set.setExpiration(-100, TimeUnit.SECONDS);
+        set.add(3);
+        set.setExpiration(20, TimeUnit.MINUTES);
+        set.add(6);
+
+        // when
+        set.removeExpiredEntries();
+
+        // then
+        assertThat(set.contains(2), equalTo(true));
+        assertThat(set.contains(3), equalTo(false));
+        assertThat(set.contains(6), equalTo(true));
+    }
+}