From ca708e23cd197e69e6970f34fb884acf0e9d8bc0 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 19 Feb 2017 09:06:15 +0100 Subject: [PATCH] #949 Create ExpiringSet, integrate into SessionManager --- .../fr/xephi/authme/data/SessionManager.java | 44 +++------ .../fr/xephi/authme/util/ExpiringSet.java | 97 +++++++++++++++++++ .../xephi/authme/data/SessionManagerTest.java | 57 ++++------- .../fr/xephi/authme/util/ExpiringSetTest.java | 91 +++++++++++++++++ 4 files changed, 220 insertions(+), 69 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/util/ExpiringSet.java create mode 100644 src/test/java/fr/xephi/authme/util/ExpiringSetTest.java diff --git a/src/main/java/fr/xephi/authme/data/SessionManager.java b/src/main/java/fr/xephi/authme/data/SessionManager.java index f86f54214..23da3afff 100644 --- a/src/main/java/fr/xephi/authme/data/SessionManager.java +++ b/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.Map; -import java.util.concurrent.ConcurrentHashMap; - -import static fr.xephi.authme.util.Utils.MILLIS_PER_MINUTE; +import java.util.concurrent.TimeUnit; /** * 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 Map sessions = new ConcurrentHashMap<>(); - + private final ExpiringSet sessions; 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) { - Long timeout = sessions.get(name.toLowerCase()); - if (timeout != null) { - return System.currentTimeMillis() <= timeout; - } - } - return false; + return enabled && sessions.contains(name.toLowerCase()); } /** @@ -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.put(name.toLowerCase(), timeout); + sessions.add(name.toLowerCase()); } } @@ -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) { - return; - } - final long currentTime = System.currentTimeMillis(); - Iterator> iterator = sessions.entrySet().iterator(); - while (iterator.hasNext()) { - Map.Entry entry = iterator.next(); - if (entry.getValue() < currentTime) { - iterator.remove(); - } + if (enabled) { + sessions.removeExpiredEntries(); } } } diff --git a/src/main/java/fr/xephi/authme/util/ExpiringSet.java b/src/main/java/fr/xephi/authme/util/ExpiringSet.java new file mode 100644 index 000000000..840d5911c --- /dev/null +++ b/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. + *

+ * 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 the type of the entries + */ +public class ExpiringSet { + + private Map 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(); + } +} diff --git a/src/test/java/fr/xephi/authme/data/SessionManagerTest.java b/src/test/java/fr/xephi/authme/data/SessionManagerTest.java index f01311af1..50b181783 100644 --- a/src/test/java/fr/xephi/authme/data/SessionManagerTest.java +++ b/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 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 sessions = getSessionsMap(manager); - sessions.put("somebody", System.currentTimeMillis() - 123L); - sessions.put("someone", System.currentTimeMillis() + 4040L); - sessions.put("anyone", System.currentTimeMillis() - 1000L); - sessions.put("everyone", System.currentTimeMillis() + 60000L); + ExpiringSet expiringSet = mockExpiringSet(); + setSessionsMap(manager, expiringSet); // when manager.performCleanup(); // then - assertThat(sessions, aMapWithSize(2)); - assertThat(sessions.keySet(), containsInAnyOrder("someone", "everyone")); + verify(expiringSet).removeExpiredEntries(); } @Test @@ -148,23 +124,28 @@ public class SessionManagerTest { // given Settings settings = mockSettings(false, 1); SessionManager manager = new SessionManager(settings); - Map sessions = getSessionsMap(manager); - sessions.put("somebody", System.currentTimeMillis() - 123L); - sessions.put("someone", System.currentTimeMillis() + 4040L); - sessions.put("anyone", System.currentTimeMillis() - 1000L); - sessions.put("everyone", System.currentTimeMillis() + 60000L); + ExpiringSet expiringSet = mockExpiringSet(); + setSessionsMap(manager, expiringSet); // when manager.performCleanup(); // then - assertThat(sessions, aMapWithSize(4)); // map not changed -> no cleanup performed + verify(expiringSet, never()).removeExpiredEntries(); } - private static Map getSessionsMap(SessionManager manager) { + private static ExpiringSet getSessionsMap(SessionManager manager) { return ReflectionTestUtils.getFieldValue(SessionManager.class, manager, "sessions"); } + private static void setSessionsMap(SessionManager manager, ExpiringSet sessionsMap) { + ReflectionTestUtils.setField(SessionManager.class, manager, "sessions", sessionsMap); + } + + @SuppressWarnings("unchecked") + private static ExpiringSet mockExpiringSet() { + return mock(ExpiringSet.class); + } private static Settings mockSettings(boolean isEnabled, int sessionTimeout) { Settings settings = mock(Settings.class); diff --git a/src/test/java/fr/xephi/authme/util/ExpiringSetTest.java b/src/test/java/fr/xephi/authme/util/ExpiringSetTest.java new file mode 100644 index 000000000..f58e03122 --- /dev/null +++ b/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 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 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 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 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 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)); + } +}