On Thu, Dec 13, 2012 at 7:26 AM, Chris Hegarty <chris.hega...@oracle.com>wrote:
> I think what you suggested should work. But how about a simpler version?
>
>
>
> if (expected > count) {
> long nskip = expected - count;
> if (nskip <= available()) {
> - long n = 0;
> - while (n < nskip) {
> - nskip = nskip - n;
> - n = skip(nskip);
> + while (nskip > 0 && available() > 0) {
>
> + skip(nskip);
> + nskip = expected - count;
> }
>
>
I was trying to avoid ever calling skip with a larger argument than
available(), trying to obey the comment
// Do this ONLY if the skip won't block.
So my version feels safer, although again I don't have the full context.
>
> -Chris
>
>
> On 12/13/2012 12:49 AM, Martin Buchholz wrote:
>
>>
>>
>> On Tue, Dec 11, 2012 at 7:40 AM, Chris Hegarty <chris.hega...@oracle.com
>> <mailto:chris.hegarty@oracle.**com <chris.hega...@oracle.com>>> wrote:
>>
>> Hi Martin,
>>
>> Thank you for reporting this issue. I filed 8004863: "Infinite Loop
>> in KeepAliveStream", to track it.
>>
>> I put together a small test to reproduce the problem (inline below).
>> It is racey, but shows the problem most of the time on my machine.
>>
>> I tried your suggested patch, but found that there were cases where
>> not enough data was being read off the stream, when it was being
>> closed. This causes a problem for subsequent requests on that
>> stream. The suggested patch below resolves this, and should also
>> resolve the problem you are seeing ( patch against jdk8 ).
>>
>> Is this something that you want to run with, or would you prefer for
>> me to get it into jdk8?
>>
>> diff -r fda257689786
>> src/share/classes/sun/net/www/**__http/KeepAliveStream.java
>> --- a/src/share/classes/sun/net/__**www/http/KeepAliveStream.java
>>
>> Mon Dec 10 10:52:11 2012 +0900
>> +++ b/src/share/classes/sun/net/__**www/http/KeepAliveStream.java
>>
>> Tue Dec 11 15:30:50 2012 +0000
>> @@ -83,10 +83,9 @@ class KeepAliveStream extends MeteredStr
>> if (expected > count) {
>> long nskip = expected - count;
>>
>> if (nskip <= available()) {
>> - long n = 0;
>> - while (n < nskip) {
>> - nskip = nskip - n;
>> - n = skip(nskip);
>> + while (nskip > 0) {
>> + skip(nskip);
>> + nskip = expected - count;
>> }
>>
>>
>> I'm still having a hard time understanding the code. It looks to me
>> like the above still has issues if there is an asynchronous close in
>> this loop. In that case skip() will make no progress and always return
>> 0, and we will continue to see nskip > 0.
>>
>> How about (completely untested)
>>
>> if (expected > count) {
>> long nskip = (long) (expected - count);
>> if (nskip <= available()) {
>> - long n = 0;
>> - while (n < nskip) {
>> - nskip = nskip - n;
>> - n = skip(nskip);
>> - }
>> + do {} while ((nskip = (long) (expected - count)) > 0L
>> + && skip(Math.min(nskip, available()))
>> > 0L);
>> } else if (expected <=
>> KeepAliveStreamCleaner.MAX_**DATA_REMAINING && !hurried) {
>> //put this KeepAliveStream on the queue so that
>> the data remaining
>> //on the socket can be cleanup asyncronously.
>>
>>
