/hg/icedtea-web: Improved CacheEntry locking system to respect t...
jkang at icedtea.classpath.org
jkang at icedtea.classpath.org
Wed Aug 20 19:53:15 UTC 2014
changeset c45cd47e962b in /hg/icedtea-web
details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=c45cd47e962b
author: Jie Kang <jkang at redhat.com>
date: Wed Aug 20 15:49:58 2014 -0400
Improved CacheEntry locking system to respect threads and processes.
2014-08-20 Jie Kang <jkang at redhat.com
Fixed CacheEntry locking system to respect threads and processes.
* netx/net/sourceforge/jnlp/cache/CacheEntry.java
(lock), (unlock): now uses PropertiesFile lock instead of CacheUtil
(tryLock), (isHeldByCurrentThread): added functions
* netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java: moved unlock
calls into finally blocks
* netx/net/sourceforge/jnlp/cache/CacheUtil.java: lock and unlock
no longer needed and are removed
* netx/net/sourceforge/jnlp/util/PropertiesFile.java: now uses LockedFile
and has methods to lock/trylock/unlock.
* netx/net/sourceforge/jnlp/util/LockedFile.java
(tryLock), (isHeldByCurrentThread): added functions
(unlock): no longer attempts unlock unless lock is held by current thread
and process
* tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java: 6 new
tests for the locking mechanism (testLock), (testUnlock),
(testStoreFailsWithoutLock), (testStoreWorksWithLock),
(testMultithreadLockPreventsWrite), (testMultithreadLockAllowsRead)
* tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java:
moved unlock calls into finally blocks
diffstat:
ChangeLog | 23 +
netx/net/sourceforge/jnlp/cache/CacheEntry.java | 24 +-
netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java | 6 +-
netx/net/sourceforge/jnlp/cache/CacheUtil.java | 194 ++++-----
netx/net/sourceforge/jnlp/util/PropertiesFile.java | 51 ++-
netx/net/sourceforge/jnlp/util/lockingfile/LockedFile.java | 19 +-
tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java | 160 ++++++++-
tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java | 18 +-
8 files changed, 354 insertions(+), 141 deletions(-)
diffs (truncated from 785 to 500 lines):
diff -r 0191d719163b -r c45cd47e962b ChangeLog
--- a/ChangeLog Wed Aug 20 10:37:44 2014 -0400
+++ b/ChangeLog Wed Aug 20 15:49:58 2014 -0400
@@ -1,3 +1,26 @@
+2014-08-20 Jie Kang <jkang at redhat.com>
+
+ Improved CacheEntry locking system to respect threads and processes.
+ * netx/net/sourceforge/jnlp/cache/CacheEntry.java
+ (lock), (unlock): now uses PropertiesFile lock instead of CacheUtil
+ (tryLock), (isHeldByCurrentThread): added functions
+ * netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java: moved unlock
+ calls into finally blocks
+ * netx/net/sourceforge/jnlp/cache/CacheUtil.java: lock and unlock
+ no longer needed and are removed
+ * netx/net/sourceforge/jnlp/util/PropertiesFile.java: now uses LockedFile
+ and has methods to lock/trylock/unlock.
+ * netx/net/sourceforge/jnlp/util/LockedFile.java
+ (tryLock), (isHeldByCurrentThread): added functions
+ (unlock): no longer attempts unlock unless lock is held by current thread
+ and process
+ * tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java: 6 new
+ tests for the locking mechanism (testLock), (testUnlock),
+ (testStoreFailsWithoutLock), (testStoreWorksWithLock),
+ (testMultithreadLockPreventsWrite), (testMultithreadLockAllowsRead)
+ * tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java:
+ moved unlock calls into finally blocks
+
2014-08-20 Jie Kang <jkang at redhat.com>
Renamed Resource Status Enum fields to better describe their meaning.
diff -r 0191d719163b -r c45cd47e962b netx/net/sourceforge/jnlp/cache/CacheEntry.java
--- a/netx/net/sourceforge/jnlp/cache/CacheEntry.java Wed Aug 20 10:37:44 2014 -0400
+++ b/netx/net/sourceforge/jnlp/cache/CacheEntry.java Wed Aug 20 15:49:58 2014 -0400
@@ -20,6 +20,7 @@
import java.io.File;
import java.net.URL;
+
import net.sourceforge.jnlp.Version;
import net.sourceforge.jnlp.util.PropertiesFile;
import net.sourceforge.jnlp.util.logging.OutputController;
@@ -211,9 +212,16 @@
/**
* Save the current information for the cache entry.
+ *
+ * @return True if successfuly stored into file, false otherwise
*/
- protected void store() {
- properties.store();
+ protected boolean store() {
+ if (properties.isHeldByCurrentThread()) {
+ properties.store();
+ return true;
+ } else {
+ return false;
+ }
}
/**
@@ -227,14 +235,22 @@
* Lock cache item.
*/
protected void lock() {
- CacheUtil.lockFile(properties);
+ properties.lock();
}
/**
* Unlock cache item.
*/
protected void unlock() {
- CacheUtil.unlockFile(properties);
+ properties.unlock();
+ }
+
+ protected boolean tryLock() {
+ return properties.tryLock();
+ }
+
+ protected boolean isHeldByCurrentThread() {
+ return properties.isHeldByCurrentThread();
}
}
diff -r 0191d719163b -r c45cd47e962b netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
--- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Aug 20 10:37:44 2014 -0400
+++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Aug 20 15:49:58 2014 -0400
@@ -36,8 +36,7 @@
*/
package net.sourceforge.jnlp.cache;
-import java.util.Set;
-import static net.sourceforge.jnlp.runtime.Translator.R;
+import static net.sourceforge.jnlp.runtime.Translator.R;
import java.io.File;
import java.io.IOException;
@@ -49,12 +48,13 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map.Entry;
+import java.util.Set;
import net.sourceforge.jnlp.config.DeploymentConfiguration;
import net.sourceforge.jnlp.runtime.JNLPRuntime;
import net.sourceforge.jnlp.util.FileUtils;
+import net.sourceforge.jnlp.util.PropertiesFile;
import net.sourceforge.jnlp.util.logging.OutputController;
-import net.sourceforge.jnlp.util.PropertiesFile;
/**
* This class helps maintain the ordering of most recently use items across
diff -r 0191d719163b -r c45cd47e962b netx/net/sourceforge/jnlp/cache/CacheUtil.java
--- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Aug 20 10:37:44 2014 -0400
+++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Aug 20 15:49:58 2014 -0400
@@ -21,7 +21,6 @@
import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
import java.io.File;
-import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.FilePermission;
import java.io.IOException;
@@ -33,10 +32,8 @@
import java.net.URLConnection;
import java.nio.channels.FileChannel;
import java.nio.channels.FileLock;
-import java.nio.channels.OverlappingFileLockException;
import java.security.Permission;
import java.util.ArrayList;
-import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map.Entry;
@@ -50,7 +47,6 @@
import net.sourceforge.jnlp.runtime.JNLPRuntime;
import net.sourceforge.jnlp.util.FileUtils;
import net.sourceforge.jnlp.util.PropertiesFile;
-import net.sourceforge.jnlp.util.UrlUtils;
import net.sourceforge.jnlp.util.logging.OutputController;
/**
@@ -65,7 +61,6 @@
private static final String setCacheDir = JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
private static final String cacheDir = new File(setCacheDir != null ? setCacheDir : System.getProperty("java.io.tmpdir")).getPath(); // Do this with file to standardize it.
private static final CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
- private static final HashMap<String, FileLock> propertiesLockPool = new HashMap<String, FileLock>();
/**
* Caches a resource and returns a URL for it in the cache;
@@ -276,17 +271,20 @@
File cacheFile = null;
synchronized (lruHandler) {
- lruHandler.lock();
+ try {
+ lruHandler.lock();
- // We need to reload the cacheOrder file each time
- // since another plugin/javaws instance may have updated it.
- lruHandler.load();
- cacheFile = getCacheFileIfExist(urlToPath(source, ""));
- if (cacheFile == null) { // We did not find a copy of it.
- cacheFile = makeNewCacheFile(source, version);
- } else
- lruHandler.store();
- lruHandler.unlock();
+ // We need to reload the cacheOrder file each time
+ // since another plugin/javaws instance may have updated it.
+ lruHandler.load();
+ cacheFile = getCacheFileIfExist(urlToPath(source, ""));
+ if (cacheFile == null) { // We did not find a copy of it.
+ cacheFile = makeNewCacheFile(source, version);
+ } else
+ lruHandler.store();
+ } finally {
+ lruHandler.unlock();
+ }
}
return cacheFile;
}
@@ -355,31 +353,33 @@
*/
public static File makeNewCacheFile(URL source, Version version) {
synchronized (lruHandler) {
- lruHandler.lock();
- lruHandler.load();
+ File cacheFile = null;
+ try {
+ lruHandler.lock();
+ lruHandler.load();
+ for (long i = 0; i < Long.MAX_VALUE; i++) {
+ String path = cacheDir + File.separator + i;
+ File cDir = new File(path);
+ if (!cDir.exists()) {
+ // We can use this directory.
+ try {
+ cacheFile = urlToPath(source, path);
+ FileUtils.createParentDir(cacheFile);
+ File pf = new File(cacheFile.getPath() + ".info");
+ FileUtils.createRestrictedFile(pf, true); // Create the info file for marking later.
+ lruHandler.addEntry(lruHandler.generateKey(cacheFile.getPath()), cacheFile.getPath());
+ } catch (IOException ioe) {
+ OutputController.getLogger().log(ioe);
+ }
- File cacheFile = null;
- for (long i = 0; i < Long.MAX_VALUE; i++) {
- String path = cacheDir + File.separator + i;
- File cDir = new File(path);
- if (!cDir.exists()) {
- // We can use this directory.
- try {
- cacheFile = urlToPath(source, path);
- FileUtils.createParentDir(cacheFile);
- File pf = new File(cacheFile.getPath() + ".info");
- FileUtils.createRestrictedFile(pf, true); // Create the info file for marking later.
- lruHandler.addEntry(lruHandler.generateKey(cacheFile.getPath()), cacheFile.getPath());
- } catch (IOException ioe) {
- OutputController.getLogger().log(ioe);
+ break;
}
+ }
- break;
- }
+ lruHandler.store();
+ } finally {
+ lruHandler.unlock();
}
-
- lruHandler.store();
- lruHandler.unlock();
return cacheFile;
}
}
@@ -530,30 +530,31 @@
* This will remove all old cache items.
*/
public static void cleanCache() {
-
if (okToClearCache()) {
// First we want to figure out which stuff we need to delete.
HashSet<String> keep = new HashSet<>();
HashSet<String> remove = new HashSet<>();
- lruHandler.load();
-
- long maxSize = -1; // Default
try {
- maxSize = Long.parseLong(JNLPRuntime.getConfiguration().getProperty("deployment.cache.max.size"));
- } catch (NumberFormatException nfe) {
- }
-
- maxSize = maxSize << 20; // Convert from megabyte to byte (Negative values will be considered unlimited.)
- long curSize = 0;
+ lruHandler.lock();
+ lruHandler.load();
- for (Entry<String, String> e : lruHandler.getLRUSortedEntries()) {
- // Check if the item is contained in cacheOrder.
- final String key = e.getKey();
- final String path = e.getValue();
+ long maxSize = -1; // Default
+ try {
+ maxSize = Long.parseLong(JNLPRuntime.getConfiguration().getProperty("deployment.cache.max.size"));
+ } catch (NumberFormatException nfe) {
+ }
- File file = new File(path);
- PropertiesFile pf = new PropertiesFile(new File(path + ".info"));
- boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
+ maxSize = maxSize << 20; // Convert from megabyte to byte (Negative values will be considered unlimited.)
+ long curSize = 0;
+
+ for (Entry<String, String> e : lruHandler.getLRUSortedEntries()) {
+ // Check if the item is contained in cacheOrder.
+ final String key = e.getKey();
+ final String path = e.getValue();
+
+ File file = new File(path);
+ PropertiesFile pf = new PropertiesFile(new File(path + ".info"));
+ boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
/*
* This will get me the root directory specific to this cache item.
@@ -563,14 +564,14 @@
* rStr first becomes: /0/http/www.example.com/subdir/a.jar
* then rstr becomes: /home/user1/.icedtea/cache/0
*/
- String rStr = file.getPath().substring(cacheDir.length());
- rStr = cacheDir + rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
- long len = file.length();
+ String rStr = file.getPath().substring(cacheDir.length());
+ rStr = cacheDir + rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
+ long len = file.length();
- if (keep.contains(file.getPath().substring(rStr.length()))) {
- lruHandler.removeEntry(key);
- continue;
- }
+ if (keep.contains(file.getPath().substring(rStr.length()))) {
+ lruHandler.removeEntry(key);
+ continue;
+ }
/*
* we remove entries from our lru if any of the following condition is met.
@@ -580,30 +581,31 @@
* - maxSize >= 0 && curSize + len > maxSize: If a limit was set and the new size
* on disk would exceed the maximum size.
*/
- if (delete || !file.isFile() || (maxSize >= 0 && curSize + len > maxSize)) {
- lruHandler.removeEntry(key);
- remove.add(rStr);
- continue;
- }
-
- curSize += len;
- keep.add(file.getPath().substring(rStr.length()));
-
- for (File f : file.getParentFile().listFiles()) {
- if (!(f.equals(file) || f.equals(pf.getStoreFile()))) {
- try {
- FileUtils.recursiveDelete(f, f);
- } catch (IOException e1) {
- OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e1);
- }
+ if (delete || !file.isFile() || (maxSize >= 0 && curSize + len > maxSize)) {
+ lruHandler.removeEntry(key);
+ remove.add(rStr);
+ continue;
}
+ curSize += len;
+ keep.add(file.getPath().substring(rStr.length()));
+
+ for (File f : file.getParentFile().listFiles()) {
+ if (!(f.equals(file) || f.equals(pf.getStoreFile()))) {
+ try {
+ FileUtils.recursiveDelete(f, f);
+ } catch (IOException e1) {
+ OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e1);
+ }
+ }
+
+ }
}
+ lruHandler.store();
+ } finally {
+ lruHandler.unlock();
}
- lruHandler.store();
-
removeSetOfDirectories(remove);
-
}
}
@@ -616,38 +618,4 @@
}
}
}
-
- /**
- * Lock the property file and add it to our pool of locks.
- *
- * @param properties Property file to lock.
- */
- public static void lockFile(PropertiesFile properties) {
- String storeFilePath = properties.getStoreFile().getPath();
- try {
- propertiesLockPool.put(storeFilePath, FileUtils.getFileLock(storeFilePath, false, true));
- } catch (OverlappingFileLockException e) {
- } catch (FileNotFoundException e) {
- OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
- }
- }
-
- /**
- * Unlock the property file and remove it from our pool. Nothing happens if
- * it wasn't locked.
- *
- * @param properties Property file to unlock.
- */
- public static void unlockFile(PropertiesFile properties) {
- File storeFile = properties.getStoreFile();
- FileLock fl = propertiesLockPool.get(storeFile.getPath());
- try {
- if (fl == null) return;
- fl.release();
- fl.channel().close();
- propertiesLockPool.remove(storeFile.getPath());
- } catch (IOException e) {
- OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
- }
- }
}
diff -r 0191d719163b -r c45cd47e962b netx/net/sourceforge/jnlp/util/PropertiesFile.java
--- a/netx/net/sourceforge/jnlp/util/PropertiesFile.java Wed Aug 20 10:37:44 2014 -0400
+++ b/netx/net/sourceforge/jnlp/util/PropertiesFile.java Wed Aug 20 15:49:58 2014 -0400
@@ -16,9 +16,15 @@
package net.sourceforge.jnlp.util;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+
+import net.sourceforge.jnlp.util.lockingfile.LockedFile;
import net.sourceforge.jnlp.util.logging.OutputController;
-import java.io.*;
-import java.util.*;
/**
* A properties object backed by a specified file without throwing
@@ -32,7 +38,7 @@
public class PropertiesFile extends Properties {
/** the file to save to */
- File file;
+ LockedFile lockedFile;
/** the header string */
String header = "netx file";
@@ -46,7 +52,7 @@
* @param file the file to save and load to
*/
public PropertiesFile(File file) {
- this.file = file;
+ this.lockedFile = LockedFile.getInstance(file);
}
/**
@@ -56,7 +62,7 @@
* @param header the file header
*/
public PropertiesFile(File file, String header) {
- this.file = file;
+ this.lockedFile = LockedFile.getInstance(file);
this.header = header;
}
@@ -98,7 +104,7 @@
* Returns the file backing this properties object.
*/
public File getStoreFile() {
- return file;
+ return lockedFile.getFile();
}
/**
@@ -110,7 +116,7 @@
* false, if file was still current
*/
public boolean load() {
-
+ File file = lockedFile.getFile();
if (!file.exists()) {
return false;
}
@@ -150,7 +156,7 @@
* Saves the properties to the file.
*/
public void store() {
-
+ File file = lockedFile.getFile();
FileOutputStream s = null;
try {
try {
@@ -169,4 +175,33 @@
}
}
+ public void lock() {
+ try {
+ lockedFile.lock();
+ } catch (IOException e) {
+ OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
+ }
+ }
+
+ public boolean tryLock() {
+ try {
+ return lockedFile.tryLock();
+ } catch (IOException e) {
+ OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
+ }
+ return false;
+ }
+
+ public void unlock() {
+ try {
+ lockedFile.unlock();
+ } catch (IOException e) {
+ OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
+ }
More information about the distro-pkg-dev
mailing list