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.riggs at oracle.com]
> *Sent:* Mittwoch, 18. Oktober 2017 17:28
> *To:* Langer, Christoph <christoph.langer at sap.com>; 
> net-dev at 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-bounces at openjdk.java.net] *On
>     Behalf Of *Roger Riggs
>     *Sent:* Dienstag, 17. Oktober 2017 16:53
>     *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,
>
>     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/21911bc9/attachment-0001.html>