RFR: 6543126: Level.known can leak memory

[Daniel Fuchs]

Hi Peter, Mandy,

On 20/08/16 21:17, Mandy Chung wrote:
> It is a good observation.  Pinning on a class loader is a good solution. On the other hand, the Level API isn’t well designed for customization (so does LogManager or Logger) that should be re-examined at some point in the future (which is one cause of the current complicated implementation).
>
> I think typical pattern would hold a strong reference to the custom level instances for the application/library use and so this compatibility risk may be relatively low.  IMO, in the current state of logging API, it’s the subclass’s reponsibility to ensure the custom level instances are not garbage collected, like loggers (where LogManager::getLogger may return null if a logger is GC’ed).  I think the right thing to do is to define a proper API for customization as a long-term solution.  As for this issue, I don’t have a strong opinion in both approaches (using ClassLoaderValue vs Daniel’s current fix).
>
> Peter - in any case, as we have identified several usages for it, I suggest to file a separate bug to move ClassLoaderValue to a jdk.internal.xxx package. jdk.internal.loader may be a better home for it.

Peter, thanks for doing the groundwork and putting
ClassLoaderValue in jdk.internal.loader.
I would have been reluctant to using it in this fix otherwise.

Here is a new webrev that uses ClassLoaderValue as Peter suggested.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.04/

I had to introduce a new private static class (KnownLevelCLV)
to avoid using lambdas in KnownLevel.add - as CustomLevel.java
was failing with an InternalError raised in MethodHandles$Lookup
otherwise.

The CustomLevel test is now sadly a bit more complex, as we have
to introduce a new ClassLoader that can be garbage collected in
order to test the fix.

Otherwise the logic remains the same compared to webrev.03.

best regards, and thanks for all the feedback!

-- daniel


>
> Mandy
>
>> On Aug 17, 2016, at 4:20 AM, Peter Levart <peter.levart at gmail.com> 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") ...
>>
>>
>> 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
>>>>
>>>
>>
>