[rfc][icedtea-web] Simple process-safe thread storage utility class
Jiri Vanek
jvanek at redhat.com
Mon Jan 21 02:28:43 PST 2013
On 01/18/2013 07:23 PM, Adam Domurad wrote:
> Posting early iteration of storage class as Jiri requested, feedback appreciated. This will be needed in coming whitelist/blacklist for unsigned applets.
Good utility class, I'm looking forward to use it.
however thre looks to be many faults, I think most of them was not in original patch I have seen. See the comments, maybe I'm wrong :)
>
> No change log atm sorry.
>
> Comments, extra testing ideas, etc.
> Locking file still needs a separate unit test
>
> -Adam
>
>
> LockingStringListStorage.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/util/LockingFile.java b/netx/net/sourceforge/jnlp/util/LockingFile.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/util/LockingFile.java
> @@ -0,0 +1,71 @@
> +package net.sourceforge.jnlp.util;
> +
> +import java.io.File;
> +import java.io.IOException;
> +import java.nio.channels.FileLock;
> +import java.util.Map;
> +import java.util.WeakHashMap;
> +import java.util.concurrent.locks.Lock;
> +import java.util.concurrent.locks.ReentrantLock;
> +
> +// Process& thread locked access to a file. Creates file if it does not already exist.
> +public class LockingFile {
> +
> + // The file for access
> + private File file;
> +
> + // A file lock will protect against locks for multiple
> + // processes, while a thread lock is still needed within a single JVM.
> +
> + private FileLock processLock = null;
> + private Lock threadLock = new ReentrantLock();
> +
> + private LockingFile(File file) {
> + this.file = file;
> +
> + try {
> + // Create if does not already exist
> + this.file.createNewFile();
> + } catch (IOException e) {
> + // Avoid propagating IOException, this shouldn't fail
> + e.printStackTrace();
No no no :) throw Throw THROW!
(or at least rethrow as runtime)
btw, why this createNewFile() ?
/me assuem that without physical file FileUtils.getFileLock does not work.
> + }
> + }
> +
> + // Provide shared access to LockedFile's via weak map
> + static private final Map<String, LockingFile> weakLockedFileCache = new WeakHashMap<String, LockingFile>();
This should be
Map<File, LockingFile> as eg my/file and file are equals (in case of working directory my) for file, but not for string.
> +
> + synchronized public static LockingFile getInstance(File file) {
> + String path = file.getAbsolutePath();
> +
> + if (!weakLockedFileCache.containsKey(path)) {
> + weakLockedFileCache.put(path, new LockingFile(file));
> + }
> +
> + return weakLockedFileCache.get(file.getAbsolutePath());
> + }
I don't like this getInstance idea:(
In previous concept was put/remove to/from map done during lock/unlock. I can not find benefits in this new concept (except confusing that user have tu use newInstance of something what is not singleton).
expected flame back here :)
> +
> + // Get the file in question
> + public File getFile() {
> + return file;
> + }
> +
> + public void lock() throws IOException {
> + threadLock.lock();
> + processLock = FileUtils.getFileLock(this.file.getPath(), false, true);
> + }
> +
> + public void unlock() {
> + threadLock.unlock();
> + try {
> + if (processLock != null) {
> + processLock.release();
> + }
> + } catch (IOException e) {
> + // We should not treat IO problems as a critical failure
> + e.printStackTrace();
no, No, NO, throw Throw THROW!-)
> + } finally {
> + processLock = null;
> + }
> + }
> +}
> \ No newline at end of file
> diff --git a/netx/net/sourceforge/jnlp/util/LockingStringListStorage.java b/netx/net/sourceforge/jnlp/util/LockingStringListStorage.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/util/LockingStringListStorage.java
> @@ -0,0 +1,128 @@
> +package net.sourceforge.jnlp.util;
> +
> +import java.io.BufferedReader;
> +import java.io.File;
> +import java.io.FileInputStream;
> +import java.io.FileOutputStream;
> +import java.io.IOException;
> +import java.io.InputStreamReader;
> +import java.io.OutputStreamWriter;
> +import java.io.PrintWriter;
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +// Process-locked storage backed by a file.
Public javadoc will be better;)
> +public class LockingStringListStorage {
> +
> + private LockingFile lockedFile;
> + private List<String> cachedContents = new ArrayList<String>();
> +
> + public LockingStringListStorage(String location) {
> + this.lockedFile = LockingFile.getInstance(new File(location));
> + }
> +
> + public File getBackingFile() {
> + return this.lockedFile.getFile();
> + }
> +
> + public void lock() throws IOException {
> + lockedFile.lock();
> + }
> +
> + public void unlock() {
> + lockedFile.unlock();
> + }
> +
> + /*
> + * Copy between memory<-> file. Assumes lock held.
> + */
> + private void writeContents() throws IOException {
> + PrintWriter writer = null;
> + try {
> + writer = new PrintWriter(new OutputStreamWriter(
> + new FileOutputStream(getBackingFile()), "UTF-8"));
> + for (String string : cachedContents) {
> + writer.println(string);
> + }
> + } finally {
> + if (writer != null) {
> + writer.close();
> + }
> + }
> + }
Pintwriter have no advantage on BufferedWriter. It have one serious advantage in more - it consumes exception. Please, use BufferedWriter.
> +
> + private void readContents() throws IOException {
I would rather see both write and read in protected scope.
> + BufferedReader reader = null;
> + try {
> + reader = new BufferedReader(new InputStreamReader(
> + new FileInputStream(getBackingFile()), "UTF-8"));
> + String line;
> + this.cachedContents.clear();
Balh!
> + while ((line = reader.readLine()) != null) {
usage of while(true){
line = reader.readLine()
if (line==null) {break}
...
Is much more simple and you avoid pre-block declaration.
> + if (line.trim().length() != 0) {
> + this.cachedContents.add(line);
hmhmh... I can imagine usages of this class which would like empty lines. Maybe make this check conditional and based on constructor param?
maybe not... Do as you wish, this is not serious blocker :)
> + }
> + }
> + } finally {
> + if (reader != null) {
> + reader.close();
> + }
> + }
> + }
> +
> + /*
> + * Atomic container abstraction methods. These all allow the file-backed
> + * string list to be treated as a convenient in-memory object.
> + */
> + synchronized public void add(String line) throws IOException {
> + lock();
> + try {
> + readContents();
> + cachedContents.add(line);
> + writeContents();
> + } finally {
> + unlock();
> + }
> + }
Ah now its little bit clearer why to lock here and not in readContents/writeContents methods themselves.
hmm... However I think it is worthy to have aslo read/write locked.
What about some counting lock?
File will be unlocekd only when counter will be less or equal zero. So:
+ lock(); //counter ++
+ try {
+ readContents(); inside will be counter ++
+ cachedContents.add(line);
+ writeContents(); inside will be counter --
+ } finally {
+ unlock(); //counter --
+ }
And maybe enchace:
> + private void read/writeContents() throws IOException {
with its twin:
private void read/writeContentsWithLocks() throws IOException {
lock()
try{
read/writeContents
finally{
unlock()
}
}
What do you think?
> +
> + synchronized public String[] toArray() throws IOException {
> + lock();
> + try {
> + readContents();
> + return cachedContents.toArray(new String[] {});
> + } finally {
> + unlock();
> + }
> + }
I would probably like to have (readOnly?) access to cachedContents
> +
> + synchronized public boolean contains(String line) throws IOException {
> + lock();
> + try {
> + readContents();
> + return cachedContents.contains(line);
> + } finally {
> + unlock();
> + }
> + }
> +
> + synchronized public void clear(String line) throws IOException {
> + lock();
> + try {
> + cachedContents.clear();
> + writeContents();
> + } finally {
> + unlock();
> + }
> + }
> +
> + synchronized public void remove(String line) throws IOException {
> + lock();
> + try {
> + readContents();
> + cachedContents.remove(line);
> + writeContents();
> + } finally {
> + unlock();
> + }
> + }
It would be nice to have all those add/clear/remove... methods withoutDeclared exceptions:
+ lock();
+ try {
+ readContents();
+ cachedContents.remove(line);
+ writeContents();
+ catch(IoException eee){ //means all declared exceptions
+ throw new RuntimeException(eeee);
+ }
+ } finally {
+ unlock(); //AFAIK should be reatched also with this rethrow
+ }
> +}
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/util/LockingStringListStorageTest.java b/tests/netx/unit/net/sourceforge/jnlp/util/LockingStringListStorageTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/LockingStringListStorageTest.java
> @@ -0,0 +1,45 @@
> +package net.sourceforge.jnlp.util;
> +
> +import static org.junit.Assert.assertFalse;
> +import static org.junit.Assert.assertTrue;
> +
> +import java.io.File;
> +import java.io.IOException;
> +import org.junit.Before;
> +import org.junit.Test;
> +
> +public class LockingStringListStorageTest {
> +
> + private File storagefile;
> +
> + private LockingStringListStorage newInstance() {
> + return new LockingStringListStorage(storagefile.getPath());
> + }
> +
> + @Before
> + public void setUp() throws IOException {
> + storagefile = File.createTempFile("foo","bar");
> + }
> +
> + @Test
> + public void testSimpleActions() throws IOException {
> + LockingStringListStorage storage = newInstance();
> +
> + storage.add("teststring");
> + assertTrue(storage.contains("teststring"));
> + storage.remove("teststring");
> + assertFalse(storage.contains("teststring"));
> + }
> +
> + @Test
> + public void testInterleavedActions() throws IOException {
> + LockingStringListStorage storage1 = newInstance();
> + LockingStringListStorage storage2 = newInstance();
> +
> + storage1.add("teststring");
> + assertTrue(storage2.contains("teststring"));
> + storage2.remove("teststring");
> + assertFalse(storage1.contains("teststring"));
> + }
I really miss some high concurrency test here. (imho the only test which have sense here :)
Some eg three threads trying to add same list (looong!) of values. Always have if !containst then add. And as assert all items must be unique in final list
(it will be good to try (probably also kept in test) that in non sync list will be mess at the end....)
> +
> +}
Sorry for more troubles and especially delaying you from better work , but as low level utility class it should be as good designed as possible.
As nitpicks - missing headers and javadoc texts.
Looking forward for this utility class!
J.
More information about the distro-pkg-dev
mailing list