Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
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
Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
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
Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
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. 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
Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
On 08/21/2013 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. Well, you will be leaking like 100-200 bytes. But any way is fine. -JB- 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
Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
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
Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
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
Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
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? Also, is the 2020: if (wr != null) { 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
Re: Codereview requested: 6566891 RMIConnector: map value referencing map key in WeakHashMap prevents map entry to be removed
Thanks Daniel and Erik for the comments, here is new version: http://cr.openjdk.java.net/~sjiang/JDK-6566891/01/ 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