From 4b7c8bb0cba29997623a0e32fb65c2e2770f476e Mon Sep 17 00:00:00 2001 From: Amos Ng Date: Fri, 12 Jun 2020 20:54:40 +0800 Subject: [PATCH 1/5] Rectification attempt at cache migration issues with nio File --- src/main/java/mdnet/cache/DiskLruCache.java | 34 ++++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/main/java/mdnet/cache/DiskLruCache.java b/src/main/java/mdnet/cache/DiskLruCache.java index 2b86b0c..5154493 100644 --- a/src/main/java/mdnet/cache/DiskLruCache.java +++ b/src/main/java/mdnet/cache/DiskLruCache.java @@ -34,6 +34,10 @@ import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Writer; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.FileAlreadyExistsException; import java.util.ArrayList; import java.util.Iterator; import java.util.LinkedHashMap; @@ -994,15 +998,35 @@ public final class DiskLruCache implements Closeable { public File getCleanFile(int i) { // Move files to new caching tree if exists - File old_cache = new File(directory, key + "." + i); - File new_cache = new File(directory + subkeypath, key + "." + i); - if (old_cache.exists()) { - old_cache.renameTo(new_cache); + Path old_cache = Paths.get(directory + File.separator + key + "." + i); + Path new_cache = Paths.get(directory + subkeypath + File.separator + key + "." + i); + if (Files.exists(old_cache)) { + try { + Files.move(old_cache, new_cache); + } catch (FileAlreadyExistsException faee) { + try { + Files.delete(old_cache); + } catch (IOException ex) {} + } catch (IOException ex) {} } - return new_cache; + + return new File(directory + subkeypath, key + "." + i); } public File getDirtyFile(int i) { + // Move files to new caching tree if exists + Path old_cache = Paths.get(directory + File.separator + key + "." + i + ".tmp"); + Path new_cache = Paths.get(directory + subkeypath + File.separator + key + "." + i + ".tmp"); + if (Files.exists(old_cache)) { + try { + Files.move(old_cache, new_cache); + } catch (FileAlreadyExistsException faee) { + try { + Files.delete(old_cache); + } catch (IOException ex) {} + } catch (IOException ex) {} + } + return new File(directory + subkeypath, key + "." + i + ".tmp"); } } From d611f4a6daa2e64ea1dd4907daef2e30b204426d Mon Sep 17 00:00:00 2001 From: Amos Ng Date: Fri, 12 Jun 2020 22:02:14 +0800 Subject: [PATCH 2/5] Use atomic operations for cache migration --- src/main/java/mdnet/cache/DiskLruCache.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/mdnet/cache/DiskLruCache.java b/src/main/java/mdnet/cache/DiskLruCache.java index 5154493..9334518 100644 --- a/src/main/java/mdnet/cache/DiskLruCache.java +++ b/src/main/java/mdnet/cache/DiskLruCache.java @@ -38,6 +38,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.FileAlreadyExistsException; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Iterator; import java.util.LinkedHashMap; @@ -1002,7 +1003,7 @@ public final class DiskLruCache implements Closeable { Path new_cache = Paths.get(directory + subkeypath + File.separator + key + "." + i); if (Files.exists(old_cache)) { try { - Files.move(old_cache, new_cache); + Files.move(old_cache, new_cache, StandardCopyOption.ATOMIC_MOVE); } catch (FileAlreadyExistsException faee) { try { Files.delete(old_cache); @@ -1019,7 +1020,7 @@ public final class DiskLruCache implements Closeable { Path new_cache = Paths.get(directory + subkeypath + File.separator + key + "." + i + ".tmp"); if (Files.exists(old_cache)) { try { - Files.move(old_cache, new_cache); + Files.move(old_cache, new_cache, StandardCopyOption.ATOMIC_MOVE); } catch (FileAlreadyExistsException faee) { try { Files.delete(old_cache); From 3a7876d492ed728d77ac94fc46dbba99a24c0bf7 Mon Sep 17 00:00:00 2001 From: Amos Ng Date: Fri, 12 Jun 2020 22:03:21 +0800 Subject: [PATCH 3/5] Fixed semantics --- src/main/java/mdnet/cache/DiskLruCache.java | 28 ++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/main/java/mdnet/cache/DiskLruCache.java b/src/main/java/mdnet/cache/DiskLruCache.java index 9334518..5108c9b 100644 --- a/src/main/java/mdnet/cache/DiskLruCache.java +++ b/src/main/java/mdnet/cache/DiskLruCache.java @@ -951,7 +951,7 @@ public final class DiskLruCache implements Closeable { private final long[] lengths; /** Subkey pathing for cache files. */ - private final String subkeypath; + private final String subKeyPath; /** True if this entry has ever been published. */ private boolean readable; @@ -967,7 +967,7 @@ public final class DiskLruCache implements Closeable { this.lengths = new long[valueCount]; // Splits the keys into a list of two characters, and join it together to use it for sub-directorying - this.subkeypath = File.separator + String.join(File.separator, key.substring(0, 8).replaceAll("..(?!$)", "$0 ").split(" ")); + this.subKeyPath = File.separator + String.join(File.separator, key.substring(0, 8).replaceAll("..(?!$)", "$0 ").split(" ")); } public String getLengths() { @@ -999,36 +999,36 @@ public final class DiskLruCache implements Closeable { public File getCleanFile(int i) { // Move files to new caching tree if exists - Path old_cache = Paths.get(directory + File.separator + key + "." + i); - Path new_cache = Paths.get(directory + subkeypath + File.separator + key + "." + i); - if (Files.exists(old_cache)) { + Path oldCache = Paths.get(directory + File.separator + key + "." + i); + Path newCache = Paths.get(directory + subKeyPath + File.separator + key + "." + i); + if (Files.exists(oldCache)) { try { - Files.move(old_cache, new_cache, StandardCopyOption.ATOMIC_MOVE); + Files.move(oldCache, newCache, StandardCopyOption.ATOMIC_MOVE); } catch (FileAlreadyExistsException faee) { try { - Files.delete(old_cache); + Files.delete(oldCache); } catch (IOException ex) {} } catch (IOException ex) {} } - return new File(directory + subkeypath, key + "." + i); + return new File(directory + subKeyPath, key + "." + i); } public File getDirtyFile(int i) { // Move files to new caching tree if exists - Path old_cache = Paths.get(directory + File.separator + key + "." + i + ".tmp"); - Path new_cache = Paths.get(directory + subkeypath + File.separator + key + "." + i + ".tmp"); - if (Files.exists(old_cache)) { + Path oldCache = Paths.get(directory + File.separator + key + "." + i + ".tmp"); + Path newCache = Paths.get(directory + subKeyPath + File.separator + key + "." + i + ".tmp"); + if (Files.exists(oldCache)) { try { - Files.move(old_cache, new_cache, StandardCopyOption.ATOMIC_MOVE); + Files.move(oldCache, newCache, StandardCopyOption.ATOMIC_MOVE); } catch (FileAlreadyExistsException faee) { try { - Files.delete(old_cache); + Files.delete(oldCache); } catch (IOException ex) {} } catch (IOException ex) {} } - return new File(directory + subkeypath, key + "." + i + ".tmp"); + return new File(directory + subKeyPath, key + "." + i + ".tmp"); } } } From 2d756d7c7cd4ce85a2d26d6cc0ea8d281ad6fdba Mon Sep 17 00:00:00 2001 From: Amos Ng Date: Fri, 12 Jun 2020 22:03:46 +0800 Subject: [PATCH 4/5] Updated CHANGELOG.md to reflect atomicness of migrator --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da80bda..19f6be8 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added - [2020-06-12] Added CHANGELOG.md by [@lflare]. -- [2020-06-12] Added on-read image migrator to 4-deep subdirectory format by [@lflare]. +- [2020-06-12] Added on-read atomic image migrator to 4-deep subdirectory format by [@lflare]. ### Changed - [2020-06-12] Raised ApacheClient socket limit to `2**18` by [@lflare]. From c302bf6d67615b71034903f2a37b2017b69aadd0 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Fri, 12 Jun 2020 09:50:49 -0500 Subject: [PATCH 5/5] Improve DiskLruCache --- src/main/java/mdnet/cache/DiskLruCache.java | 105 +++++++------------- 1 file changed, 37 insertions(+), 68 deletions(-) diff --git a/src/main/java/mdnet/cache/DiskLruCache.java b/src/main/java/mdnet/cache/DiskLruCache.java index 5108c9b..5e2715b 100644 --- a/src/main/java/mdnet/cache/DiskLruCache.java +++ b/src/main/java/mdnet/cache/DiskLruCache.java @@ -82,9 +82,10 @@ import java.util.regex.Pattern; *
  • When an entry is being edited, it is not necessary to * supply data for every value; values default to their previous value. * - * Every {@link #editImpl} call must be matched by a call to {@link Editor#commit} - * or {@link Editor#abort}. Committing is atomic: a read observes the full set - * of values as they were before or after the commit, but never a mix of values. + * Every {@link #editImpl} call must be matched by a call to + * {@link Editor#commit} or {@link Editor#abort}. Committing is atomic: a read + * observes the full set of values as they were before or after the commit, but + * never a mix of values. * *

    * Clients call {@link #get} to read a snapshot of an entry. The read will @@ -110,7 +111,6 @@ public final class DiskLruCache implements Closeable { private static final long ANY_SEQUENCE_NUMBER = -1; public static final Pattern LEGAL_KEY_PATTERN = Pattern.compile("[a-z0-9_-]{1,120}"); - public static final Pattern UNSAFE_LEGAL_KEY_PATTERN = Pattern.compile("[a-z0-9_-][\\/a-z0-9_-]{0,119}"); private static final String CLEAN = "CLEAN"; private static final String DIRTY = "DIRTY"; @@ -412,16 +412,6 @@ public final class DiskLruCache implements Closeable { return getImpl(key); } - /** - * Returns a snapshot of the entry named {@code key}, or null if it doesn't - * exist is not currently readable. If a value is returned, it is moved to the - * head of the LRU queue. Unsafe as it allows arbitrary directories to be accessed! - */ - public Snapshot getUnsafe(String key) throws IOException { - validateUnsafeKey(key); - return getImpl(key); - } - public synchronized Snapshot getImpl(String key) throws IOException { checkNotClosed(); Entry entry = lruEntries.get(key); @@ -474,15 +464,6 @@ public final class DiskLruCache implements Closeable { return editImpl(key, ANY_SEQUENCE_NUMBER); } - /** - * Returns an editor for the entry named {@code key}, or null if another edit is - * in progress. Unsafe as it allows arbitrary directories to be accessed! - */ - public Editor editUnsafe(String key) throws IOException { - validateUnsafeKey(key); - return editImpl(key, ANY_SEQUENCE_NUMBER); - } - private synchronized Editor editImpl(String key, long expectedSequenceNumber) throws IOException { checkNotClosed(); Entry entry = lruEntries.get(key); @@ -614,17 +595,6 @@ public final class DiskLruCache implements Closeable { return removeImpl(key); } - /** - * Drops the entry for {@code key} if it exists and can be removed. Entries - * actively being edited cannot be removed. Unsafe as it allows arbitrary directories to be accessed! - * - * @return true if an entry was removed. - */ - public boolean removeUnsafe(String key) throws IOException { - validateUnsafeKey(key); - return removeImpl(key); - } - private synchronized boolean removeImpl(String key) throws IOException { checkNotClosed(); Entry entry = lruEntries.get(key); @@ -709,13 +679,6 @@ public final class DiskLruCache implements Closeable { } } - private void validateUnsafeKey(String key) { - Matcher matcher = UNSAFE_LEGAL_KEY_PATTERN.matcher(key); - if (!matcher.matches()) { - throw new IllegalArgumentException("keys must match regex " + UNSAFE_LEGAL_KEY_PATTERN + ": \"" + key + "\""); - } - } - /** A snapshot of the values for an entry. */ public final class Snapshot implements Closeable { private final String key; @@ -790,7 +753,7 @@ public final class DiskLruCache implements Closeable { * Returns an unbuffered input stream to read the last committed value, or null * if no value has been committed. */ - public InputStream newInputStream(int index) { + public synchronized InputStream newInputStream(int index) { synchronized (DiskLruCache.this) { if (entry.currentEditor != this) { throw new IllegalStateException(); @@ -806,32 +769,13 @@ public final class DiskLruCache implements Closeable { } } - /** - * Returns the last committed value as a string, or null if no value has been - * committed. - */ - public String getString(int index) throws IOException { - try (InputStream in = newInputStream(index)) { - return in != null ? IOUtils.toString(in, StandardCharsets.UTF_8) : null; - } - } - - /** - * Write a string to the specified index. - */ - public void setString(int index, String value) throws IOException { - try (OutputStream out = newOutputStream(index)) { - IOUtils.write(value, out, StandardCharsets.UTF_8); - } - } - /** * Returns a new unbuffered output stream to write the value at {@code index}. * If the underlying output stream encounters errors when writing to the * filesystem, this edit will be aborted when {@link #commit} is called. The * returned output stream does not throw IOExceptions. */ - public OutputStream newOutputStream(int index) { + public synchronized OutputStream newOutputStream(int index) { if (index < 0 || index >= valueCount) { throw new IllegalArgumentException("Expected index " + index + " to " + "be greater than 0 and less than the maximum value count " + "of " + valueCount); @@ -862,6 +806,25 @@ public final class DiskLruCache implements Closeable { } } + /** + * Returns the last committed value as a string, or null if no value has been + * committed. + */ + public String getString(int index) throws IOException { + try (InputStream in = newInputStream(index)) { + return in != null ? IOUtils.toString(in, StandardCharsets.UTF_8) : null; + } + } + + /** + * Write a string to the specified index. + */ + public void setString(int index, String value) throws IOException { + try (OutputStream out = newOutputStream(index)) { + IOUtils.write(value, out, StandardCharsets.UTF_8); + } + } + /** * Commits this edit so it is visible to readers. This releases the edit lock so * another edit may be started on the same key. @@ -966,8 +929,10 @@ public final class DiskLruCache implements Closeable { this.key = key; this.lengths = new long[valueCount]; - // Splits the keys into a list of two characters, and join it together to use it for sub-directorying - this.subKeyPath = File.separator + String.join(File.separator, key.substring(0, 8).replaceAll("..(?!$)", "$0 ").split(" ")); + // Splits the keys into a list of two characters, and join it together to use it + // for sub-directorying + this.subKeyPath = File.separator + + String.join(File.separator, key.substring(0, 8).replaceAll("..(?!$)", "$0 ").split(" ")); } public String getLengths() { @@ -1007,8 +972,10 @@ public final class DiskLruCache implements Closeable { } catch (FileAlreadyExistsException faee) { try { Files.delete(oldCache); - } catch (IOException ex) {} - } catch (IOException ex) {} + } catch (IOException ex) { + } + } catch (IOException ex) { + } } return new File(directory + subKeyPath, key + "." + i); @@ -1024,8 +991,10 @@ public final class DiskLruCache implements Closeable { } catch (FileAlreadyExistsException faee) { try { Files.delete(oldCache); - } catch (IOException ex) {} - } catch (IOException ex) {} + } catch (IOException ex) { + } + } catch (IOException ex) { + } } return new File(directory + subKeyPath, key + "." + i + ".tmp");