Re: [jira] Created: (HARMONY-6675) Reducing timeout value in CanonicalPatchCache to fix a file not found error in Hadoop common

[Regis]

On 2010-11-18 4:05, sebb wrote:

Just had a look at the code.

Effectively the timeout field is a public mutable static variable.
Access to the field is not synchronised at all.

So if one thread sets a new value, other threads may or may not see
the updated value (ever).
Worse, unless the JVM guarantees that writes to long variables are
atomic, AFAICT if two threads write the value, then it's possible that
the timeout field could end up with a value that is equal to neither
setting.

Making the field volatile should fix these problems - I think.

Yes, setTimeout is not treahd safe here, in my understanding, volatile could 
make sure threads read the latest value, but to resolve write conflict, 
synchronization is still needed.


There is a subtle bug - if the property sets the timeout to<=0, then
isEnable is set to false. setTimeout() never sets it true.

Furthermore, since access to isEnable is not synch. either, if one
thread sets timeout 0 and the another to non-zero, isValid could end
up being incorrect for the value of the timeout. It's always risky
when two fields are interdependent like this.

It would be safer to eliminate the isValid field and just check the
value of the timeout.

True, field 'isEnable' is not necessary, will remove it.


BTW, the private instance fields in CacheElement should be final; that
would make the class immutable and thread-safe.


Go through the code, I also make field 'cache', 'list' and 'lock' to final.

So [1] is a new patch according Sebb's comments:

Index: 
modules/luni/src/main/java/org/apache/harmony/luni/internal/io/FileCanonPathCache.java
=====================================================================
--- 
modules/luni/src/main/java/org/apache/harmony/luni/internal/io/FileCanonPathCache.java
+++ 
modules/luni/src/main/java/org/apache/harmony/luni/internal/io/FileCanonPathCache.java
@@ -29,9 +29,9 @@ import java.util.LinkedList;
 public class FileCanonPathCache {

     static private class CacheElement {
-        String canonicalPath;
+        final String canonicalPath;

-        long timestamp;
+        final long timestamp;

         public CacheElement(String path) {
             this.canonicalPath = path;
@@ -44,25 +44,20 @@ public class FileCanonPathCache {
      */
     public static final int CACHE_SIZE = 256;

-    private static HashMap<String, CacheElement> cache = new HashMap<String, 
CacheElement>(
+    private static final HashMap<String, CacheElement> cache = new 
HashMap<String, CacheElement>(
             CACHE_SIZE);

     /**
      * FIFO queue for tracking age of elements.
      */
-    private static LinkedList<String> list = new LinkedList<String>();
+    private static final LinkedList<String> list = new LinkedList<String>();

-    private static Object lock = new Object();
+    private static final Object lock = new Object();

     /**
      * Expired time, 0 disable this cache.
      */
-    private static long timeout = 30000;
-
-    /**
-     * Whether to enable this cache.
-     */
-    private static boolean isEnable = true;
+    private static volatile long timeout = 30000;

     public static final String FILE_CANONICAL_PATH_CACHE_TIMEOUT = 
"org.apache.harmony.file.canonical.path.cache.timeout";

@@ -74,8 +69,8 @@ public class FileCanonPathCache {
             // use default timeout value
         }

-        if (timeout <= 0) {
-            isEnable = false;
+        if (timeout < 0) {
+            timeout = 0;
         }
     }

@@ -88,7 +83,8 @@ public class FileCanonPathCache {
      *
      */
     public static String get(String path) {
-        if (!isEnable) {
+        long localTimeout = timeout;
+        if (localTimeout == 0) {
             return null;
         }

@@ -102,7 +98,7 @@ public class FileCanonPathCache {
         }

         long time = System.currentTimeMillis();
-        if (time - element.timestamp > timeout) {
+        if (time - element.timestamp > localTimeout) {
             // remove all elements older than this one
             synchronized (lock) {
                 if (cache.get(path) != null) {
@@ -128,7 +124,7 @@ public class FileCanonPathCache {
      *            the canonical path of <code>path</code>.
      */
     public static void put(String path, String canonicalPath) {
-        if (!isEnable) {
+        if (timeout == 0) {
             return;
         }

@@ -148,7 +144,7 @@ public class FileCanonPathCache {
      * Remove all elements from cache.
      */
     public static void clear() {
-        if (!isEnable) {
+        if (timeout == 0) {
             return;
         }

@@ -163,10 +159,12 @@ public class FileCanonPathCache {
     }

     public static void setTimeout(long timeout) {
-        FileCanonPathCache.timeout = timeout;
-        if (timeout <= 0) {
-            clear();
-            isEnable = false;
+        synchronized (lock) {
+            if (timeout < 0) {
+                timeout = 0;
+                clear();
+            }
+            FileCanonPathCache.timeout = timeout;
         }
     }
 }

--
Best Regards,
Regis.