BufferedInputStream#fill() hanging in SocketInputStream#read0()

[Andreas Kohn]

Hi,

in an application that makes heavy use of JAX-RS, and for that reason
small HTTP connections, I frequently observe situations where
BufferedInputStream would hang like this:

"ae03a305-557e-4db6-b9b8-2bf50f056aaf" prio=10 tid=0x00002aab0175a000 
nid=0x30dc runnable [0x0000000048cb3000]
   java.lang.Thread.State: RUNNABLE
        at java.net.SocketInputStream.socketRead0(Native Method)
        at java.net.SocketInputStream.read(SocketInputStream.java:129)
        at java.io.BufferedInputStream.fill(BufferedInputStream.java:218)
        at java.io.BufferedInputStream.read1(BufferedInputStream.java:258)
        at java.io.BufferedInputStream.read(BufferedInputStream.java:317)
        - locked <0x00002aaab605b670> (a java.io.BufferedInputStream)
        at sun.net.www.http.HttpClient.parseHTTPHeader(HttpClient.java:687)
        at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:632)
        at 
sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1072)

Typically when looking into those hangs with a debugger it seems that the HTTP 
response
was received fully, but the BufferedIInputStream tries to fill its buffer with 
more bytes than
were originally requested by the HttpClient. The HTTP server is now waiting for 
the client to
continue sending requests, while the client is stuck, hoping that the server 
will send more bytes
to fill its buffer with.

Currently the only option I have in this case is restart the application (even 
killing the server
does not lead to a connection reset for some reason!), which in many cases is 
quite unacceptable.
Disabling Keep-Alive connections helps if the hanging occurs when reading 
response bodies, but not
when the hanging occurs while the header is being read.


I believe this essentially the issue 6192696 (BufferedInputStream.read(byte[], 
int, int)
can block if the entire buffer can't be filled). I understand that this issue 
had had multiple
fix-attempts, which could not be done due reliance on a proper and performing 
#available() implementation. 

But, wouldn't it be enough to just prevent fill() from filling more than the 
user wanted to
read in the first place? For the HTTP example the Content-Length gives that 
information, and the
server will not send more than that, but on the other hand it is safe for the 
connection to block
until those bytes are received.

Attached is a patch that implements that idea, am I missing something here? 
Note that I left #read()
without arguments alone.
I've been running with this patch for a few weeks now in test environments, and 
it seems to have no
negative impact on classloading speed as detailed in the original bug comment 
trail.

Regards,
--
Andreas

-- 
Never attribute to malice that which can be adequately explained by
stupidity.                                        -- Hanlon's Razor

# HG changeset patch
# Parent 9421efd75671f60e334a0ea9d9f5bd00028065d3
Only try to fill the buffer of a buffered inputstream to the length given in the call to #skip()/#read()

Original bugs:
6192696: (depended on #available(), and killed ZipInputStream performance)

diff --git a/src/share/classes/java/io/BufferedInputStream.java b/src/share/classes/java/io/BufferedInputStream.java
--- a/src/share/classes/java/io/BufferedInputStream.java
+++ b/src/share/classes/java/io/BufferedInputStream.java
@@ -203,6 +203,10 @@ class BufferedInputStream extends Filter
      * hence pos > count.
      */
     private void fill() throws IOException {
+        fill(Integer.MAX_VALUE);
+    }
+    
+    private void fill(int max) throws IOException {
         byte[] buffer = getBufIfOpen();
         if (markpos < 0)
             pos = 0;            /* no mark: throw away the buffer */
@@ -232,7 +236,7 @@ class BufferedInputStream extends Filter
                 buffer = nbuf;
             }
         count = pos;
-        int n = getInIfOpen().read(buffer, pos, buffer.length - pos);
+        int n = getInIfOpen().read(buffer, pos, Math.min(max, buffer.length - pos));
         if (n > 0)
             count = n + pos;
     }
@@ -272,7 +276,7 @@ class BufferedInputStream extends Filter
             if (len >= getBufIfOpen().length && markpos < 0) {
                 return getInIfOpen().read(b, off, len);
             }
-            fill();
+            fill(len);
             avail = count - pos;
             if (avail <= 0) return -1;
         }
@@ -366,7 +370,7 @@ class BufferedInputStream extends Filter
                 return getInIfOpen().skip(n);
 
             // Fill in buffer to save bytes for reset
-            fill();
+            fill(n > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int) n);
             avail = count - pos;
             if (avail <= 0)
                 return 0;

signature.asc
Description: This is a digitally signed message part