Hi Roger, thanks for looking. So you motivated me to do some more cleanup. :) Here is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8155590.1/ I replaced the Stack with an ArrayDeque and did some more rather cosmetical changes to make KeepAliveCache more appealing. I verified the change by running the jtreg tests jdk/sun/net/www/http/* and it all passes.
As for the CCE: I don't think this should be checked as the Stack/ArrayDeque is typed to KeepAliveEntry and no code appears to be in place which could trick this. Best regards Christoph From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Roger Riggs Sent: Dienstag, 17. Oktober 2017 16:53 To: net-dev@openjdk.java.net Subject: Re: RFR(S): 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache Hi, Keep the synchronized, it will be low overhead since the Vector operations are synchronized and in the same thread. I think a CCE could occur during the iteration to find the entry when Vector.Itr.next() checks. (It you want to more radical fix, replace the Vector with something more current. It would be one less Vector). Roger On 10/16/17 2:33 AM, Langer, Christoph wrote: Hi Vyom, thanks for your feedback. I'm not so sure about dropping "synchronized". In the new remove method of ClientVector we are iterating ourself. If this is not done under synchronization, there is risk to run into a ConcurrentModificationException. But under the assumption that all access to ClientVector comes from synchronized methods of KeepAliveCache, one could argue to drop all synchronized modifiers for ClientVector, though. Let's wait for other opinions :) Best regards Christoph From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of vyom tewari Sent: Montag, 16. Oktober 2017 10:27 To: net-dev@openjdk.java.net<mailto:net-dev@openjdk.java.net> Subject: Re: RFR(S): 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache Hi Christoph, Thanks for doing this, i think you don't need to synchronize the "remove(HttpClient h)". This remove is get called from synchronize "remove (HttpClient h, Object obj)" and the underline data structure is which is java.util.Vector(ClientVector extends java.util.Stack) is also thread safe. What do you think ? Thanks, Vyom On Monday 16 October 2017 12:52 PM, Langer, Christoph wrote: Hi, Here is a proposal for a fix for bug 8155590. I already made this fix a while ago in our JDK clone and I'd like to contribute this. Bug: https://bugs.openjdk.java.net/browse/JDK-8155590 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8155590.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8155590.0/> Please review. Thanks Christoph
