On 18 November 2010 03:01, Regis <[email protected]> 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.
