[9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Mandy Chung]

What I wanted to say is that you are creating a new CacheKey which is used to load a resource bundle and if found, it will be put in the cacheList.

It seems wrong to continue the request for loading a bundle but its caller module and target module is GC’ed and put a NONEXISTENT_BUNDLE in the cache. I consider it as an error state if we ever get there.

Mandy

> On Jan 13, 2017, at 4:54 PM, Naoto Sato <naoto.sato at oracle.com> wrote:
> 
> Modified as:
> 
> diff -r a6d3c80ea436 src/java.base/share/classes/java/util/ResourceBundle.java
> --- a/src/java.base/share/classes/java/util/ResourceBundle.java
> +++ b/src/java.base/share/classes/java/util/ResourceBundle.java
> @@ -2192,7 +2192,7 @@
>     private static void clearCacheImpl(Module callerModule, ClassLoader loader) {
>         cacheList.keySet().removeIf(
>             key -> key.getCallerModule() == callerModule &&
> -                   getLoader(key.getModule()) == loader
> +                   key.getModule() != null && getLoader(key.getModule()) == loader
>         );
>     }
> 
> This is the only occurence where CacheKey's modules are passed to getLoader(Module). Entire webrev is here:
> 
> http://cr.openjdk.java.net/~naoto/8171139/webrev.05/
> 
> Naoto
> 
> On 01/13/2017 04:25 PM, Mandy Chung wrote:
>> 
>>> On Jan 13, 2017, at 4:05 PM, Naoto Sato <naoto.sato at oracle.com> wrote:
>>>> 
>>>> Daniel asks whether there is any possibility that the target module or caller module would be GC’ed.
>>>> 
>>>> CacheKey is copied in two places, one in findBundle and the other is putBundleInCache which is actually passed with a newly cloned CacheKey.  IIUC, the cacheKey passed to findBundle ia always a new instance (as it subsequently sets to different locale during the lookup).  Its caller module is the caller on the stack and the target module is also a strong reference either caller’s module, unnamed module from a given class loader, or a given module.
>>>> 
>>>> Naoto will have to double check.
>>> 
>>> I think this is correct. The current way of using CacheKey is safe from its modules to be GC'ed. However in general, it'd be good prepare them to be GC'ed. I modified the impl to hold them in local variables preventing them to be GC'ed before instantiating new References. (Also I modified not to call the other constructor in the copy constructor, reinstating some piece what Peter's diff originally had). So here is the updated webrev:
>>> 
>>> http://cr.openjdk.java.net/~naoto/8171139/webrev.04/
>> 
>> So callerRef and moduleRef may be null. getLoader(null) may be called which will throw NPE.
>> 
>> Mandy
>> 
>