RFR(S): 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache

[Roger Riggs]

Hi Christoph,

Looks ok.

The comment in remove() about CCE can be removed.  ArrayDeque is not 
thread safe
and doesn't check for CCE (and the method is synchronized).

Thanks, Roger


On 10/18/17 6:05 AM, Langer, Christoph wrote:
>
> Hi Roger,
>
> thanks for looking. So you motivated me to do some more cleanup. J
>
> Here is the new webrev: 
> http://cr.openjdk.java.net/~clanger/webrevs/8155590.1/ 
> <http://cr.openjdk.java.net/%7Eclanger/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-bounces at openjdk.java.net] *On Behalf Of 
> *Roger Riggs
> *Sent:* Dienstag, 17. Oktober 2017 16:53
> *To:* net-dev at 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 J
>
>     Best regards
>
>     Christoph
>
>     *From:*net-dev [mailto:net-dev-bounces at openjdk.java.net] *On
>     Behalf Of *vyom tewari
>     *Sent:* Montag, 16. Oktober 2017 10:27
>     *To:* net-dev at openjdk.java.net <mailto:net-dev at 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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20171018/1ebbb17d/attachment-0001.html>