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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph

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/

 

Please review.

 

Thanks

Christoph

 

Reply | Threaded
Open this post in threaded view
|

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

Vyom Tewari

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/

 

Please review.

 

Thanks

Christoph

 


Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph

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:[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

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

roger riggs
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 [[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph

 

 


Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph

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/

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:[hidden email]] On Behalf Of Roger Riggs
Sent: Dienstag, 17. Oktober 2017 16:53
To: [hidden email]
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 [[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph

 

 

 

Reply | Threaded
Open this post in threaded view
|

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/

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 [[hidden email]] On Behalf Of Roger Riggs
Sent: Dienstag, 17. Oktober 2017 16:53
To: [hidden email]
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 [[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph

 

 

 


Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph

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:[hidden email]]
Sent: Mittwoch, 18. Oktober 2017 17:28
To: Langer, Christoph <[hidden email]>; [hidden email]
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/

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 [[hidden email]] On Behalf Of Roger Riggs
Sent: Dienstag, 17. Oktober 2017 16:53
To: [hidden email]
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 [[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph

 

 

 

 

Reply | Threaded
Open this post in threaded view
|

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 [[hidden email]]
Sent: Mittwoch, 18. Oktober 2017 17:28
To: Langer, Christoph [hidden email]; [hidden email]
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/

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 [[hidden email]] On Behalf Of Roger Riggs
Sent: Dienstag, 17. Oktober 2017 16:53
To: [hidden email]
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 [[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph

 

 

 

 


Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph

Hi Roger,

 

ah, I see. I pushed with removing the comment then: http://hg.openjdk.java.net/jdk10/master/rev/74e1913a98c0

 

Thanks

Christoph

 

From: Roger Riggs [mailto:[hidden email]]
Sent: Mittwoch, 18. Oktober 2017 20:37
To: Langer, Christoph <[hidden email]>; [hidden email]
Subject: Re: RFR(S): 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache

 

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 [[hidden email]]
Sent: Mittwoch, 18. Oktober 2017 17:28
To: Langer, Christoph [hidden email]; [hidden email]
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/

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 [[hidden email]] On Behalf Of Roger Riggs
Sent: Dienstag, 17. Oktober 2017 16:53
To: [hidden email]
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 [[hidden email]] On Behalf Of vyom tewari
Sent: Montag, 16. Oktober 2017 10:27
To: [hidden email]
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/

 

Please review.

 

Thanks

Christoph