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

[Daniel Fuchs]

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