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

[shanliang]

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