7u Code review request for 7033170, 7092821, 7092825
Sean Mullan
sean.mullan at oracle.com
Fri Aug 7 13:16:20 UTC 2015
Hi David,
On 08/07/2015 08:44 AM, David Schlosnagle wrote:
> I forgot to mention one other workaround for this bug, as used in Guava
> is cloning the instance if clone() is supported, see
> https://github.com/google/guava/issues/1197
>
> On Friday, August 7, 2015, David Schlosnagle <schlosna at gmail.com
> <mailto:schlosna at gmail.com>> wrote:
>
> // reviving this ghost
>
> Hi Valerie, Sean, and sec-dev,
>
> I am curious if there has been any movement on incorporating a fix
> for https://bugs.openjdk.java.net/browse/JDK-7092821? I have
> encountered several systems where this is a significant contention
> and scale bottleneck. While there are some workarounds such as
> pooling and reusing Provider instances, that seems like a band-aid,
> and fixing the JDK is a better path.
I don't know if Valerie has made any progress on this issue, so I'll let
her add any details.
>
> If I were to put a webrev together, would someone be kind enough
> to sponsor it for me?
Yes, I think we could do that and I see you have already signed the OCA.
> Are there any existing open source load tests
> to verify these changes?
Not that I know of. It would be great if you could implement a JMH
microbenchmark as part of your fix.
> Is there a good mechanism in OpenJDK now to
> run JMH across all of the supported platforms (I only have access to
> a small subset of the permutation of platforms).
That's being worked on for JDK 9 but AFAIK it isn't ready yet. See
http://openjdk.java.net/jeps/230
Thanks for your help!
--Sean
>
> Thanks!
>
> - Dave
>
> On Thursday, January 12, 2012, Valerie (Yu-Ching) Peng
> <valerie.peng at oracle.com
> <javascript:_e(%7B%7D,'cvml','valerie.peng at oracle.com');>> wrote:
>
> 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 }
>
>
More information about the security-dev
mailing list