harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@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 12:05:29 GMT
On 18 November 2010 03:01, Regis <xu.regis@gmail.com> wrote:
> 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) {

That was correct.

> -            clear();
> -            isEnable = false;
> +        synchronized (lock) {
> +            if (timeout < 0) {

That is incorrect; should be

              if (timeout <= 0) {

otherwise you won't call clear() for timeout == 0.


> +                timeout = 0;
> +                clear();
> +            }
> +            FileCanonPathCache.timeout = timeout;
>         }
>     }
>  }
>

Otherwise patch looks good.

Mime
View raw message