7u Code review request for 7033170, 7092821, 7092825

[Valerie (Yu-Ching) Peng]

Dave,
Thanks for the comments.
Let me think about it some more and see how to better address this kind 
of racing issue.

If you have concerns on fixes for 7033170 and 7092825, please let me know.

Sean,
Can you please ignore the review request for 7092821 for now. I'll send 
out an updated version later.
If you can still review the remaining two, that'd be great.

Thanks,
Valerie

On 01/11/12 21:48, David Schlosnagle wrote:
> On Wed, Jan 11, 2012 at 8:04 PM, Valerie (Yu-Ching) Peng
> <valerie.peng at oracle.com>  wrote:
>> 7092821: java.security.Provider.getService() is synchronized and became scalability bottleneck.
>> jdk7u Webrev: http://cr.openjdk.java.net/~valeriep/7092821_7u/
>> jdk8 Webrev: http://cr.openjdk.java.net/~valeriep/7092821/
> Valerie,
>
> You might already be aware of this, but there is a data race on lines
> 685 - 686 of the Provider's getService(String, String) method. If
> there are concurrent callers to getService while lookupCache == null,
> the lookupCache may be overwritten by a new ConcurrentHashMap after
> another thread has just instantiated and  populated an entry in the
> cache leading to thrashing on lookupCache. It might be worthwhile to
> either use a double checked lock to avoid the race at the expense of
> an additional lock and volatile read in the case lookupCache == null
> or add a comment indicating that this is an accepted data race.
>
> There is also now the possibility that if one thread is executing
> getService while another thread invokes one of the methods that sets
> lookupCache = null, there could then be a NullPointerException on line
> 703 as the volatile read would now see a null and fail. You could
> prevent that by either moving the putIfAbsent under the lock (not
> ideal for performance if you're already seeing contention), or just
> maintain a local reference to the cache map and use it throughout the
> getService method. This would mean that if the lookupCache reference
> was concurrently mutated, the putIfAbsent would basically be a write
> to the local map reference which is now garbage, but shouldn't really
> hurt anything.
>
> I'd propose something along the lines of the following to address this:
>
>      public Service getService(String type, String algorithm) {
>          ServiceKey key = new ServiceKey(type, algorithm);
>          ConcurrentMap<ServiceKey,Service>  localLookupCache = getLookupCache();
>          Service result = localLookupCache.get(key);
>          if (result != null) {
>              return (result == NULL_MARK? null : result);
>          }
>          synchronized (this) {
>              checkInitialized();
>              if (serviceMap != null) {
>                  result = serviceMap.get(key);
>              }
>              if (result == null) {
>                  ensureLegacyParsed();
>                  result = (legacyMap != null) ? legacyMap.get(key) : null;
>              }
>          }
>          // under concurrent mutation of lookupCache, this will write
> to map that is no
>          // longer the active cache
>          localLookupCache.putIfAbsent(key, (result == null? NULL_MARK : result));
>          return result;
>      }
>
>      private ConcurrentMap<ServiceKey,Service>  getLookupCache() {
>          if (lookupCache == null) {
>              synchronized (this) {
>                  // must fall back on double checked lock
>                  if (lookupCache == null) {
>                      lookupCache = new ConcurrentHashMap<>();
>                  }
>              }
>          }
>          return lookupCache;
>      }
>
>
> - Dave
>
>
> For reference, here were the original changes:
>   429     // Cache for service lookups. Discard whenever services are changed.
>   430     private transient volatile ConcurrentMap<ServiceKey,Service>
> lookupCache;
> ...snip...
>   682     public Service getService(String type, String algorithm) {
>   683         ServiceKey key = new ServiceKey(type, algorithm);
>   684         Service result = null;
>   685         if (lookupCache == null) {
>   686             lookupCache = new ConcurrentHashMap<>();
>   687         } else {
>   688             result = lookupCache.get(key);
>   689             if (result != null) {
>   690                 return (result == NULL_MARK? null : result);
>   691             }
>   692         }
>   693         synchronized (this) {
>   694             checkInitialized();
>   695             if (serviceMap != null) {
>   696                 result = serviceMap.get(key);
>   697             }
>   698             if (result == null) {
>   699                 ensureLegacyParsed();
>   700                 result = (legacyMap != null) ? legacyMap.get(key) : null;
>   701             }
>   702         }
>   703         lookupCache.putIfAbsent(key, (result == null? NULL_MARK : result));
>   704         return result;
>   705     }