RFR: 6543126: Level.known can leak memory

[Peter Levart]

On 08/16/2016 02:30 PM, Daniel Fuchs wrote:
> Hi Peter,
>
> On 16/08/16 13:05, Peter Levart wrote:
>> Hi Daniel,
>>
>> Passing Stream<Level> returning extractors to Stream::flatMap is one way
>> of doing it. The other would be to make findByXXX methods return
>> Optional<KnownLevel> and then use Optional::map method on the result to
>> extract the needed property, so instead of:
>>
>>     Optional<Level> level = findByName(name, KnownLevel::referent);
>>
>> You could simply make findByName return Optional<KnownLevel> and then:
>>
>>     Optional<Level> level = findByName(name).map(Reference::get);
>>
>> Just plain null-when-absent-returning getters used with Optional::map
>> would do.
>
> Well - not exactly because when looking for levelObject the KnownLevel
> could become stale after it's returned. What we want is loop until
> we find a levelObject which is not null (that's why the previous
> version had some while loops in Level.parse).

Ah, I see. Right. Of course, my bad.

In that case, there is a possible race that could lead to exception here:

  476             // add new Level.
  477             Level levelObject = new Level(name, x);
  478             return KnownLevel.findByValue(x, 
KnownLevel::referent).get();

...between lines 477 and 478 and between lines 377 and 378, the newly 
constructed Level object could already get GC-ed and so findByValue 
could return an empty Optional.

You should do something like:

Level levelObject = new Level(name, x);
Level result = KnownLevel.findByValue(x, KnownLevel::referent).get();
Reference.reachabilityFence(levelObject);
return result;


Regards, Peter

>
> But maybe I'm misunderstanding what you are saying?
>
> best regards,
>
> -- daniel
>
>>
>>
>> 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
>>>>
>>>
>>
>