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

[shanliang]

Thanks David.

Shanliang

David Holmes wrote:
> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>