Re: 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-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 J

    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