jmx-dev Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed

[David Holmes]

Hi Shanliang,

After our off-list discussion I've had a closer look at this and it 
looks okay to me too.

Reviewed.

Thanks,
David

On 26/08/2013 6:35 PM, shanliang wrote:
> Hi,
>
> Still need OK from a reviewer.
> Thanks Erik, Jaroslav and Daniel for the code review.
>
> Thanks,
> Shanliang
>
> Daniel Fuchs wrote:
>> On 8/21/13 10:27 AM, shanliang wrote:
>>> Jaroslav Bachorik wrote:
>>>> On 08/21/2013 12:00 AM, Daniel Fuchs wrote:
>>>>
>>>>> On 8/20/13 11:12 PM, shanliang wrote:
>>>>>
>>>>>> Thanks Daniel and Erik for the comments, here is new version:
>>>>>>     http://cr.openjdk.java.net/~sjiang/JDK-6566891/01/
>>>>>>
>>>>> This new version looks reasonable. I guess you could dispense of
>>>>> the new
>>>>> nullSubjectConnRef by just using 'null' as key in the WeakHashMap
>>>>> (as this
>>>>> seems to be the way it  used to work before) - but I'm OK with your
>>>>> proposed changes.
>>>>>
>>>>
>>>> Indeed, the change could be simplified by just using the null key.
>>>>
>>> If using "null" as a key, then the pair null/weakRef will never be
>>> removed from WeakHashMap, even the value of weakRef is null. Of course
>>> this is a minor memory leak, or almost nothing, but it seems more clean
>>> to use nullSubjectConnRef.
>>
>> Ah. Good point. I didn't think of that...
>>
>>> Still need a reviewer.
>>>
>>> Thanks,
>>> Shanliang
>>>> The rest seems fine. Thumbs up!
>>>>
>>>> -JB-
>>>>
>>>>
>>>>> -- daniel
>>>>>
>>>>>> Erik Gahlin wrote:
>>>>>>
>>>>>>> Shanliang,
>>>>>>>
>>>>>>> Let's say the delgationSubject is null and there is previous
>>>>>>> connection cached (nullSubjectConn != null && nullSubjectConn.get()
>>>>>>> != null) so you hit:
>>>>>>>
>>>>>>> 2016: conn = nullSubjectConn.get();
>>>>>>>
>>>>>>> but before you get the object a GC occurs and clears reference, then
>>>>>>> getConnectionWithSubject would return null.
>>>>>>>
>>>>>>> Shouldn't you assign to nullSubjectConn.get() to variable so the
>>>>>>> reference is kept alive? Or it OK to return null?
>>>>>>>
>>>>>> Yes, should avoid calling 2 times.
>>>>>>
>>>>>>> Also, is the
>>>>>>>
>>>>>>> 2020: if (wr != null) {
>>>>>>>
>>>>>> This line is no more there.
>>>>>>
>>>>>> Shanliang
>>>>>>
>>>>>>> necessary?
>>>>>>>
>>>>>>> Thanks
>>>>>>> Erik
>>>>>>>
>>>>>>> nullSibwhen you hit the second if-statement
>>>>>>>   happens
>>>>>>> shanliang skrev 2013-08-20 17:35:
>>>>>>>
>>>>>>>> webrev:http://cr.openjdk.java.net/~sjiang/JDK-6566891/00/
>>>>>>>>
>>>>>>>> shanliang wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review:
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://amos.fr.oracle.com/jmgt/user/sjiang/webrevs/jdk8-6566891/00/
>>>>>>>>>
>>>>>>>>> bug:https://jbs.oracle.com/bugs/browse/JDK-6566891
>>>>>>>>>
>>>>>>>>> I have passed JCK tests and unit tests.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Shanliang
>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>