Re: 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.hega...@oracle.com
<mailto: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.