Infinite Loop in KeepAliveStream

[Chris Hegarty]

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;
                      }


-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>> 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.
>