RFR: 6543126: Level.known can leak memory

[Peter Levart]

Hi Daniel,

Thinking of this patch from the compatibility standpoint, aren't you 
afraid that someone might be using the following idiom:

public class MyLevel extends java.util.logging.Level {
     static {
         new MyLevel("LEVEL1", 1);
         new MyLevel("LEVEL2", 2);
         ...
     }

     public static void init() {}

     private MyLevel(String name, int value) {
         super(name, value);
     }
     ...
}

...and then use those levels in code like:

     MyLevel.init();
     ...
     Level level = Level.parse("LEVEL1") ...


With the proposed patch, this will not work any more as custom Level 
subclass instances will possibly be GCed before being "parsed" and retained.

To prevent that from happening and still allow custom Level subclasses 
and their class loaders to be garbage-collectable, I recommend that you 
"pin" each constructed subclass instance to its ClassLoader with a 
strong reference. You could use a ClassLoaderValue for this purpose, 
like in the following addition to your patch (see KnownLevel.add):

http://cr.openjdk.java.net/~plevart/jdk9-dev/Level.known/webrev.01/

Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:
> Hi Mandy,
>
> I added an additional selector parameter to the find methods.
> This made it possible to return Optional<Level> instead of
> KnownLevel - and it does simply the parse() method.
>
> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02
>
> best regards,
>
> -- daniel
>
> On 11/08/16 20:12, Mandy Chung wrote:
>>
>>> On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fuchs at oracle.com> 
>>> wrote:
>>>
>>> On 10/08/16 17:21, Mandy Chung wrote:
>>>>> On Jul 29, 2016, at 4:54 AM, Daniel Fuchs 
>>>>> <daniel.fuchs at oracle.com> wrote:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/
>>>> This looks pretty good.
>>>>
>>>> Since KnownLevel is now a Reference, I suggest to change 
>>>> KnownLevel::findByName, findByValue and findByLocalizedLevelName to 
>>>> return Optional<Level> instead such that the parse method 
>>>> implementaiton could be simplified.
>>>
>>> We need to return KnownLevel because sometimes we need the
>>> level object and sometimes the mirror.
>>
>> So either findByName(String name, boolean mirror) or two methods: 
>> findLevelByName and findMirroredLevelByName??
>>
>> Or seriously consider to remove KnownLevel class by introducing a new 
>> Level subclass with final Level.getName, Level.getLocalizedName, 
>> Level.getResourceBundleName methods??
>>
>> Mandy
>>
>