Apply changes to file-based websession cache from HA-plugin The HA-plugin contains a modified version of the websession-flatfile plugin. The version used in the HA-plugin was updated since it was forked, but those changes were not applied to the websession-flatfile plugin itself. This change applies some minor changes made to the corresponding code in the HA-plugin to the websession-flatfile plugin. This includes: - Variable renaming - `dir` -> `websessionsDir` - `flatFileWebSessionCache` -> `cache` - `cleanupInterval` -> `cleanupIntervalMillis` - Removing `final` from parameters in `FlatFileWebSession` - Using `SECONDS.toMillis(1)` inline as initial delay for cleanup tasks instead of a class field (`INITIAL_DELAY_MS = 1000`) - Using `path.toFile().exists()` instead of `Files.exists(path)`. While minor changes, they bring the code closer to what is used in the HA-plugin, making it easier to port future changes. Change-Id: I38da3e9fc7dc2dcbada82b63d51acd595f7b93a8
diff --git a/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSession.java b/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSession.java index dac8be6..813e392 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSession.java +++ b/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSession.java
@@ -45,13 +45,13 @@ @Inject FlatFileWebSession( - @RootRelative final Provider<HttpServletRequest> request, - @RootRelative final Provider<HttpServletResponse> response, - final WebSessionManagerFactory managerFactory, - final FlatFileWebSessionCache cache, - final AuthConfig authConfig, - final Provider<AnonymousUser> anonymousProvider, - final RequestFactory identified) { + @RootRelative Provider<HttpServletRequest> request, + @RootRelative Provider<HttpServletResponse> response, + WebSessionManagerFactory managerFactory, + FlatFileWebSessionCache cache, + AuthConfig authConfig, + Provider<AnonymousUser> anonymousProvider, + RequestFactory identified) { super( request.get(), response.get(),
diff --git a/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCache.java b/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCache.java index fc6ee9b..0d29720 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCache.java +++ b/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCache.java
@@ -75,12 +75,12 @@ } } - private final Path dir; + private final Path websessionsDir; @Inject - public FlatFileWebSessionCache(@WebSessionDir Path dir) throws IOException { - this.dir = dir; - Files.createDirectories(dir); + public FlatFileWebSessionCache(@WebSessionDir Path websessionsDir) throws IOException { + this.websessionsDir = websessionsDir; + Files.createDirectories(websessionsDir); } @Override @@ -137,7 +137,7 @@ @Nullable public Val getIfPresent(Object key) { if (key instanceof String) { - Path path = dir.resolve((String) key); + Path path = websessionsDir.resolve((String) key); return readFile(path); } return null; @@ -146,7 +146,7 @@ @Override public void invalidate(Object key) { if (key instanceof String) { - deleteFile(dir.resolve((String) key)); + deleteFile(websessionsDir.resolve((String) key)); } } @@ -167,7 +167,7 @@ @Override public void put(String key, Val value) { try { - Path tempFile = Files.createTempFile(dir, UUID.randomUUID().toString(), null); + Path tempFile = Files.createTempFile(websessionsDir, UUID.randomUUID().toString(), null); try (OutputStream fileStream = Files.newOutputStream(tempFile); ObjectOutputStream objStream = new ObjectOutputStream(fileStream)) { objStream.writeObject(value); @@ -178,7 +178,7 @@ StandardCopyOption.ATOMIC_MOVE); } } catch (IOException e) { - log.warn("Cannot put into cache " + dir, e); + log.warn("Cannot put into cache " + websessionsDir, e); } } @@ -201,7 +201,7 @@ } private Val readFile(Path path) { - if (Files.exists(path)) { + if (path.toFile().exists()) { try (InputStream fileStream = Files.newInputStream(path); ObjectInputStream objStream = new ObjectInputStream(fileStream)) { return (Val) objStream.readObject(); @@ -210,12 +210,12 @@ "Entry " + path + " in cache " - + dir + + websessionsDir + " has an incompatible " + "class and can't be deserialized. Invalidating entry."); invalidate(path.getFileName().toString()); } catch (IOException e) { - log.warn("Cannot read cache " + dir, e); + log.warn("Cannot read cache " + websessionsDir, e); } } return null; @@ -225,18 +225,18 @@ try { Files.deleteIfExists(path); } catch (IOException e) { - log.error("Error trying to delete " + path + " from " + dir, e); + log.error("Error trying to delete " + path + " from " + websessionsDir, e); } } private List<Path> listFiles() { List<Path> files = new ArrayList<>(); - try (DirectoryStream<Path> dirStream = Files.newDirectoryStream(dir)) { + try (DirectoryStream<Path> dirStream = Files.newDirectoryStream(websessionsDir)) { for (Path path : dirStream) { files.add(path); } } catch (IOException e) { - log.error("Cannot list files in cache " + dir, e); + log.error("Cannot list files in cache " + websessionsDir, e); } return files; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheCleaner.java b/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheCleaner.java index 426870b..5c88f92 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheCleaner.java +++ b/src/main/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheCleaner.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.websession.flatfile; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.extensions.events.LifecycleListener; @@ -26,10 +27,10 @@ @Singleton class FlatFileWebSessionCacheCleaner implements LifecycleListener { - private static final int INITIAL_DELAY_MS = 1000; + private final WorkQueue queue; private final Provider<CleanupTask> cleanupTaskProvider; - private final long cleanupInterval; + private final long cleanupIntervalMillis; private ScheduledFuture<?> scheduledCleanupTask; static class CleanupTask implements Runnable { @@ -60,7 +61,7 @@ @CleanupInterval long cleanupInterval) { this.queue = queue; this.cleanupTaskProvider = cleanupTaskProvider; - this.cleanupInterval = cleanupInterval; + this.cleanupIntervalMillis = cleanupInterval; } @Override @@ -69,7 +70,10 @@ queue .getDefaultQueue() .scheduleAtFixedRate( - cleanupTaskProvider.get(), INITIAL_DELAY_MS, cleanupInterval, MILLISECONDS); + cleanupTaskProvider.get(), + SECONDS.toMillis(1), + cleanupIntervalMillis, + MILLISECONDS); } @Override
diff --git a/src/test/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheTest.java b/src/test/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheTest.java index c7a7fc6..22401d1 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/websession/flatfile/FlatFileWebSessionCacheTest.java
@@ -47,48 +47,48 @@ @Rule public TemporaryFolder tempFolder = new TemporaryFolder(); - private FlatFileWebSessionCache flatFileWebSessionCache; - private Path dir; + private FlatFileWebSessionCache cache; + private Path websessionDir; @Before public void createFlatFileWebSessionCache() throws Exception { - dir = tempFolder.newFolder("websessions").toPath(); - flatFileWebSessionCache = new FlatFileWebSessionCache(dir); + websessionDir = tempFolder.newFolder("websessions").toPath(); + cache = new FlatFileWebSessionCache(websessionDir); } @Test public void asMapTest() throws Exception { loadKeyToCacheDir(EMPTY_KEY); - assertThat(flatFileWebSessionCache.asMap()).isEmpty(); + assertThat(cache.asMap()).isEmpty(); loadKeyToCacheDir(INVALID_KEY); - assertThat(flatFileWebSessionCache.asMap()).isEmpty(); + assertThat(cache.asMap()).isEmpty(); loadKeyToCacheDir(EXISTING_KEY); - assertThat(flatFileWebSessionCache.asMap()).containsKey(EXISTING_KEY); + assertThat(cache.asMap()).containsKey(EXISTING_KEY); } @Test public void constructorCreateDir() throws IOException { - assertThat(dir.toFile().delete()).isTrue(); - flatFileWebSessionCache = new FlatFileWebSessionCache(dir); - assertThat(dir.toFile().exists()).isTrue(); + assertThat(websessionDir.toFile().delete()).isTrue(); + cache = new FlatFileWebSessionCache(websessionDir); + assertThat(websessionDir.toFile().exists()).isTrue(); } @Test public void cleanUpTest() throws Exception { loadKeyToCacheDir(EXISTING_KEY); try { - long existingKeyExpireAt = flatFileWebSessionCache.getIfPresent(EXISTING_KEY).getExpiresAt(); + long existingKeyExpireAt = cache.getIfPresent(EXISTING_KEY).getExpiresAt(); TimeMachine.useFixedClockAt( Instant.ofEpochMilli(existingKeyExpireAt).minus(1, ChronoUnit.HOURS)); - flatFileWebSessionCache.cleanUp(); - assertThat(isDirEmpty(dir)).isFalse(); + cache.cleanUp(); + assertThat(isDirEmpty(websessionDir)).isFalse(); TimeMachine.useFixedClockAt( Instant.ofEpochMilli(existingKeyExpireAt).plus(1, ChronoUnit.HOURS)); - flatFileWebSessionCache.cleanUp(); - assertThat(isDirEmpty(dir)).isTrue(); + cache.cleanUp(); + assertThat(isDirEmpty(websessionDir)).isTrue(); } finally { TimeMachine.useSystemDefaultZoneClock(); } @@ -97,43 +97,43 @@ @Test public void cleanUpWithErrorsWhileListingFilesTest() throws Exception { tempFolder.delete(); - flatFileWebSessionCache.cleanUp(); - assertThat(flatFileWebSessionCache.size()).isEqualTo(0); + cache.cleanUp(); + assertThat(cache.size()).isEqualTo(0); } @Test public void cleanUpWithErrorsWhileDeleteFileTest() throws Exception { loadKeyToCacheDir(EXISTING_KEY); try { - dir.toFile().setWritable(false); - flatFileWebSessionCache.cleanUp(); - assertThat(flatFileWebSessionCache.size()).isEqualTo(1); + websessionDir.toFile().setWritable(false); + cache.cleanUp(); + assertThat(cache.size()).isEqualTo(1); } finally { - dir.toFile().setWritable(true); + websessionDir.toFile().setWritable(true); } } @Test public void getIfPresentEmptyKeyTest() throws Exception { - assertThat(flatFileWebSessionCache.getIfPresent(EMPTY_KEY)).isNull(); + assertThat(cache.getIfPresent(EMPTY_KEY)).isNull(); } @Test public void getIfPresentObjectNonStringTest() throws Exception { - assertThat(flatFileWebSessionCache.getIfPresent(new Object())).isNull(); + assertThat(cache.getIfPresent(new Object())).isNull(); } @Test public void getIfPresentInvalidKeyTest() throws Exception { loadKeyToCacheDir(INVALID_KEY); - Path path = dir.resolve(INVALID_KEY); - assertThat(flatFileWebSessionCache.getIfPresent(path.toString())).isNull(); + Path path = websessionDir.resolve(INVALID_KEY); + assertThat(cache.getIfPresent(path.toString())).isNull(); } @Test public void getIfPresentTest() throws Exception { loadKeyToCacheDir(EXISTING_KEY); - assertThat(flatFileWebSessionCache.getIfPresent(EXISTING_KEY)).isNotNull(); + assertThat(cache.getIfPresent(EXISTING_KEY)).isNotNull(); } @Test @@ -142,8 +142,8 @@ loadKeyToCacheDir(INVALID_KEY); loadKeyToCacheDir(EXISTING_KEY); List<String> keys = ImmutableList.of(EMPTY_KEY, EXISTING_KEY); - assertThat(flatFileWebSessionCache.getAllPresent(keys).size()).isEqualTo(1); - assertThat(flatFileWebSessionCache.getAllPresent(keys)).containsKey(EXISTING_KEY); + assertThat(cache.getAllPresent(keys).size()).isEqualTo(1); + assertThat(cache.getAllPresent(keys)).containsKey(EXISTING_KEY); } @Test @@ -154,10 +154,10 @@ return null; } } - assertThat(flatFileWebSessionCache.get(EXISTING_KEY, new ValueLoader())).isNull(); + assertThat(cache.get(EXISTING_KEY, new ValueLoader())).isNull(); loadKeyToCacheDir(EXISTING_KEY); - assertThat(flatFileWebSessionCache.get(EXISTING_KEY, new ValueLoader())).isNotNull(); + assertThat(cache.get(EXISTING_KEY, new ValueLoader())).isNotNull(); } @Test(expected = ExecutionException.class) @@ -168,91 +168,91 @@ throw new Exception(); } } - assertThat(flatFileWebSessionCache.get(EXISTING_KEY, new ValueLoader())).isNull(); + assertThat(cache.get(EXISTING_KEY, new ValueLoader())).isNull(); } @Test public void invalidateAllCollectionTest() throws Exception { int numberOfKeys = 15; List<String> keys = loadKeysToCacheDir(numberOfKeys); - assertThat(flatFileWebSessionCache.size()).isEqualTo(numberOfKeys); - assertThat(isDirEmpty(dir)).isFalse(); + assertThat(cache.size()).isEqualTo(numberOfKeys); + assertThat(isDirEmpty(websessionDir)).isFalse(); - flatFileWebSessionCache.invalidateAll(keys); - assertThat(flatFileWebSessionCache.size()).isEqualTo(0); - assertThat(isDirEmpty(dir)).isTrue(); + cache.invalidateAll(keys); + assertThat(cache.size()).isEqualTo(0); + assertThat(isDirEmpty(websessionDir)).isTrue(); } @Test public void invalidateAllTest() throws Exception { int numberOfKeys = 5; loadKeysToCacheDir(numberOfKeys); - assertThat(flatFileWebSessionCache.size()).isEqualTo(numberOfKeys); - assertThat(isDirEmpty(dir)).isFalse(); + assertThat(cache.size()).isEqualTo(numberOfKeys); + assertThat(isDirEmpty(websessionDir)).isFalse(); - flatFileWebSessionCache.invalidateAll(); - assertThat(flatFileWebSessionCache.size()).isEqualTo(0); - assertThat(isDirEmpty(dir)).isTrue(); + cache.invalidateAll(); + assertThat(cache.size()).isEqualTo(0); + assertThat(isDirEmpty(websessionDir)).isTrue(); } @Test public void invalidateTest() throws Exception { - Path fileToDelete = Files.createFile(dir.resolve(EXISTING_KEY)); + Path fileToDelete = Files.createFile(websessionDir.resolve(EXISTING_KEY)); assertThat(Files.exists(fileToDelete)).isTrue(); - flatFileWebSessionCache.invalidate(EXISTING_KEY); + cache.invalidate(EXISTING_KEY); assertThat(Files.exists(fileToDelete)).isFalse(); } @Test public void invalidateTestObjectNotString() throws Exception { loadKeyToCacheDir(EXISTING_KEY); - assertThat(flatFileWebSessionCache.size()).isEqualTo(1); - flatFileWebSessionCache.invalidate(new Object()); - assertThat(flatFileWebSessionCache.size()).isEqualTo(1); + assertThat(cache.size()).isEqualTo(1); + cache.invalidate(new Object()); + assertThat(cache.size()).isEqualTo(1); } @Test public void putTest() throws Exception { loadKeyToCacheDir(EXISTING_KEY); - Val val = flatFileWebSessionCache.getIfPresent(EXISTING_KEY); - flatFileWebSessionCache.put(NEW_KEY, val); - assertThat(flatFileWebSessionCache.getIfPresent(NEW_KEY)).isNotNull(); + Val val = cache.getIfPresent(EXISTING_KEY); + cache.put(NEW_KEY, val); + assertThat(cache.getIfPresent(NEW_KEY)).isNotNull(); } @Test public void putAllTest() throws Exception { loadKeyToCacheDir(EXISTING_KEY); - Val val = flatFileWebSessionCache.getIfPresent(EXISTING_KEY); + Val val = cache.getIfPresent(EXISTING_KEY); Map<String, Val> sessions = ImmutableMap.of(NEW_KEY, val); - flatFileWebSessionCache.putAll(sessions); - assertThat(flatFileWebSessionCache.asMap()).containsKey(NEW_KEY); + cache.putAll(sessions); + assertThat(cache.asMap()).containsKey(NEW_KEY); } @Test public void putWithErrorsTest() throws Exception { loadKeyToCacheDir(EXISTING_KEY); - Val val = flatFileWebSessionCache.getIfPresent(EXISTING_KEY); + Val val = cache.getIfPresent(EXISTING_KEY); tempFolder.delete(); - flatFileWebSessionCache.put(NEW_KEY, val); - assertThat(flatFileWebSessionCache.getIfPresent(NEW_KEY)).isNull(); + cache.put(NEW_KEY, val); + assertThat(cache.getIfPresent(NEW_KEY)).isNull(); } @Test public void sizeTest() throws Exception { int numberOfKeys = 10; loadKeysToCacheDir(numberOfKeys); - assertThat(flatFileWebSessionCache.size()).isEqualTo(numberOfKeys); + assertThat(cache.size()).isEqualTo(numberOfKeys); } @Test public void statTest() throws Exception { - assertThat(flatFileWebSessionCache.stats()).isNull(); + assertThat(cache.stats()).isNull(); } private List<String> loadKeysToCacheDir(int number) throws IOException { List<String> keys = new ArrayList<>(); for (int i = 0; i < number; i++) { - Path tmp = Files.createTempFile(dir, "cache", null); + Path tmp = Files.createTempFile(websessionDir, "cache", null); keys.add(tmp.getFileName().toString()); } return keys; @@ -266,10 +266,10 @@ private Path loadKeyToCacheDir(String key) throws IOException { if (key.equals(EMPTY_KEY)) { - return Files.createFile(dir.resolve(EMPTY_KEY)); + return Files.createFile(websessionDir.resolve(EMPTY_KEY)); } try (InputStream in = loadFile(key)) { - Path target = dir.resolve(key); + Path target = websessionDir.resolve(key); Files.copy(in, target, StandardCopyOption.REPLACE_EXISTING); return target; }