Infinite Loop in KeepAliveStream

[Martin Buchholz]

On Thu, Dec 13, 2012 at 7:26 AM, Chris Hegarty <chris.hegarty at 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.hegarty at oracle.com
>> <mailto:chris.hegarty at oracle.**com <chris.hegarty at 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.
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/net-dev/attachments/20121214/b0c52947/attachment.html