harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Regis <xu.re...@gmail.com>
Subject Re: [jira] Created: (HARMONY-6675) Reducing timeout value in CanonicalPatchCache to fix a file not found error in Hadoop common
Date Thu, 18 Nov 2010 03:01:09 GMT
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.

Mime
View raw message