search

artiya4u / google-http-java-client

Automatically exported from code.google.com/p/google-http-java-client
0 stars 0 forks source link

FindBugs found: Synchronization issue in api.client.util.Data #24

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
Version of google-http-java-client (e.g. 1.5.0-beta)?

1.5.0-beta

Java environment (e.g. Java 6, Android 2.3, App Engine 1.4.3)?

All

Describe the problem.

This class is synchronizing on the monitor of a ConcurrentHashMap instance. 
Concurrent objects have their own concurrency controls that are distinct form 
and incompatible with the keyword synchronized. The following function will not 
prevent other threads from making changes to NULL_CHANGE because it uses the 
NULL_CHANGE monitor, while other threads may be using the built-in concurrency 
provided by the collection:

  public static <T> T nullOf(Class<?> objClass) {
    Object result = NULL_CACHE.get(objClass);
    if (result == null) {
      synchronized (NULL_CACHE) {
        result = NULL_CACHE.get(objClass);
        if (result == null) {
          if (objClass.isArray()) {
            // arrays are special because we need to compute both the dimension and component type
            int dims = 0;
            Class<?> componentType = objClass;
            do {
              componentType = componentType.getComponentType();
              dims++;
            } while (componentType.isArray());
            result = Array.newInstance(componentType, new int[dims]);
          } else if (objClass.isEnum()) {
            // enum requires look for constant with @NullValue
            FieldInfo fieldInfo = ClassInfo.of(objClass).getFieldInfo(null);
            Preconditions.checkNotNull(
                fieldInfo, "enum missing constant with @NullValue annotation: %s", objClass);
            @SuppressWarnings({"unchecked", "rawtypes"})
            Enum e = fieldInfo.<Enum>enumValue();
            result = e;
          } else {
            // other classes are simpler
            result = Types.newInstance(objClass);
          }
          NULL_CACHE.put(objClass, result);
        }
      }
    }
    @SuppressWarnings("unchecked")
    T tResult = (T) result;
    return tResult;
  }

FindBugs Output:

Synchronization performed on util.concurrent instance
This method performs synchronization an object that is an instance of a class 
from the java.util.concurrent package (or its subclasses). Instances of these 
classes have there own concurrency control mechanisms that are distinct from 
and incompatible with the use of the keyword synchronized.

How would you expect it to be fixed?

Original issue reported on code.google.com by yan...@google.com on 16 Aug 2011 at 2:23

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 28 Oct 2011 at 4:46

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 29 Feb 2012 at 1:13

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 27 Mar 2012 at 2:22

GoogleCodeExporter commented 9 years ago

Original comment by rmis...@google.com on 14 May 2012 at 2:14

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 30 May 2012 at 10:05

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 2 Aug 2012 at 2:29

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 26 Sep 2012 at 12:14

GoogleCodeExporter commented 9 years ago

Original comment by yan...@google.com on 24 Jan 2013 at 2:12

GoogleCodeExporter commented 9 years ago 
I don't think there's an actual bug here.  We picked NULL_CACHE to synchronize 
on, but we could have picked any object.  But I agree it's a bit more of a 
puzzler than it ought to be from a style point of view.  My preference would be 
to re-write it using an explicit ReentrantLock:

http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantLoc
k.html

Original comment by yan...@google.com on 9 Apr 2013 at 1:38