RFR: 6543126: Level.known can leak memory

[Daniel Fuchs]

Hi Peter,

On 17/08/16 12:20, Peter Levart wrote:
> 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") ...

That's a possibility, and maybe that would deserve to be noted as
a side effect of the fix in the release notes.

I do not believe this is something we would want to actively support
though: the usual idiom for custom level would rather be to define
them as public final static constant, and use a direct reference
in the code.

Something like:

public class MyLevel extends Level {

     public static final MyLevel LEVEL1 = new MyLevel("LEVEL1", 1);
     ...
}

    Level level = MyLevel.LEVEL1;

or even more simpler:

    public static class Levels {

       public static Level LEVEL1 = new Level("LEVEL1", 1) {};
       ...
    }


So maybe this is a risk we should take. Mandy, what do you think?

The other alternative is not to fix the bug as it's been here
for several releases ;-)

-- daniel

>
>
> 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
>>>
>>
>