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

[Roger Riggs]

Hi Christoph,

Right, my mistake, I meant CME.  My point was that ArrayDeque does not 
throw CME from remove().
It is not multi-thread safe and does not check for CME.

That remove might have been coded using the iterator:

synchronized boolean remove(HttpClient h) {
    for (Iterator<KeepAliveEntry> it =this.iterator();
         it.hasNext();) {
        KeepAliveEntry curr = it.next();
        if (curr.hc == h) {
            it.remove();
            return true;
        }
    }
    return false;
}


your choice

Thanks, Roger





On 10/18/17 9:47 AM, Langer, Christoph wrote:

Hi Roger,

maybe we have a little disconnect here in understanding. I thought you 
mean ClassCastException with CCE but I don’t see this mentioned 
anywhere. My comment talks about ConcurrentModificationException 
(CME). I mentioned that because, when the Collection is modified while 
iterating (which I do by calling super.remove()) then the next call to 
the Iterator would throw a CME. But we don’t go back to the iterator 
as we return after removing.

Nevertheless, I can still remove the comment or change it… let me know.

Thanks

Christoph

*From:*Roger Riggs [mailto:roger.ri...@oracle.com]
*Sent:* Mittwoch, 18. Oktober 2017 17:28
*To:* Langer, Christoph <christoph.lan...@sap.com>; 
net-dev@openjdk.java.net
*Subject:* Re: RFR(S): 8155590: Dubious collection management in 
sun.net.www.http.KeepAliveCache

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