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