6993490: SocketTimeoutException on HTTP keep-alive connections

Reviewed-by: michaelm

src/share/classes/sun/net/NetworkClient.java

@@ -40,6 +40,12 @@ import java.security.PrivilegedAction;
  * @author      Jonathan Payne
  */
 public class NetworkClient {
+    /* Default value of read timeout, if not specified (infinity) */
+    public static final int DEFAULT_READ_TIMEOUT = -1;
+
+    /* Default value of connect timeout, if not specified (infinity) */
+    public static final int DEFAULT_CONNECT_TIMEOUT = -1;
+
     protected Proxy     proxy = Proxy.NO_PROXY;
     /** Socket for communicating with server. */
     protected Socket    serverSocket = null;
@@ -53,8 +59,8 @@ public class NetworkClient {
     protected static int defaultSoTimeout;
     protected static int defaultConnectTimeout;
 
-    protected int readTimeout = -1;
-    protected int connectTimeout = -1;
+    protected int readTimeout = DEFAULT_READ_TIMEOUT;
+    protected int connectTimeout = DEFAULT_CONNECT_TIMEOUT;
     /* Name of encoding to use for output */
     protected static String encoding;
 
@@ -71,16 +77,12 @@ public class NetworkClient {
                         return null;
             }
         });
-        if (vals[0] == 0)
-            defaultSoTimeout = -1;
-        else
+        if (vals[0] != 0) {
             defaultSoTimeout = vals[0];
-
-        if (vals[1] == 0)
-            defaultConnectTimeout = -1;
-        else
+        }
+        if (vals[1] != 0) {
             defaultConnectTimeout = vals[1];
-
+        }
 
         encoding = encs[0];
         try {
@@ -232,7 +234,23 @@ public class NetworkClient {
         return connectTimeout;
     }
 
+    /**
+     * Sets the read timeout.
+     *
+     * Note: Public URLConnection (and protocol specific implementations)
+     * protect against negative timeout values being set. This implemenation,
+     * and protocol specific implementations, use -1 to represent the default
+     * read timeout.
+     *
+     * This method may be invoked with the default timeout value when the
+     * protocol handler is trying to reset the timeout after doing a
+     * potentially blocking internal operation, e.g. cleaning up unread
+     * response data, buffering error stream response data, etc
+     */
     public void setReadTimeout(int timeout) {
+        if (timeout == DEFAULT_READ_TIMEOUT)
+            timeout = defaultSoTimeout;
+
         if (serverSocket != null && timeout >= 0) {
             try {
                 serverSocket.setSoTimeout(timeout);

src/share/classes/sun/net/www/protocol/ftp/FtpURLConnection.java

@@ -46,6 +46,7 @@ import java.net.ProxySelector;
 import java.util.StringTokenizer;
 import java.util.Iterator;
 import java.security.Permission;
+import sun.net.NetworkClient;
 import sun.net.www.MessageHeader;
 import sun.net.www.MeteredStream;
 import sun.net.www.URLConnection;
@@ -102,11 +103,11 @@ public class FtpURLConnection extends URLConnection {
     static final int BIN = 2;
     static final int DIR = 3;
     int type = NONE;
-    /* Redefine timeouts from java.net.URLConnection as we nee -1 to mean
+    /* Redefine timeouts from java.net.URLConnection as we need -1 to mean
      * not set. This is to ensure backward compatibility.
      */
-    private int connectTimeout = -1;
-    private int readTimeout = -1;
+    private int connectTimeout = NetworkClient.DEFAULT_CONNECT_TIMEOUT;;
+    private int readTimeout = NetworkClient.DEFAULT_READ_TIMEOUT;;
 
     /**
      * For FTP URLs we need to have a special InputStream because we

src/share/classes/sun/net/www/protocol/http/HttpURLConnection.java

@@ -359,11 +359,11 @@ public class HttpURLConnection extends java.net.HttpURLConnection {
 
     private TunnelState tunnelState = TunnelState.NONE;
 
-    /* Redefine timeouts from java.net.URLConnection as we nee -1 to mean
+    /* Redefine timeouts from java.net.URLConnection as we need -1 to mean
      * not set. This is to ensure backward compatibility.
      */
-    private int connectTimeout = -1;
-    private int readTimeout = -1;
+    private int connectTimeout = NetworkClient.DEFAULT_CONNECT_TIMEOUT;
+    private int readTimeout = NetworkClient.DEFAULT_READ_TIMEOUT;
 
     /* Logging support */
     private static final PlatformLogger logger =
@@ -1041,9 +1041,9 @@ public class HttpURLConnection extends java.net.HttpURLConnection {
                     throw new ProtocolException("Server rejected operation");
                 }
             }
-            if (oldTimeout > 0) {
-                http.setReadTimeout(oldTimeout);
-            }
+
+            http.setReadTimeout(oldTimeout);
+
             responseCode = -1;
             responses.reset();
             // Proceed

test/sun/net/www/http/HttpClient/B6726695.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 6726695
+ * @bug 6726695 6993490
  * @summary HttpURLConnection shoul support 'Expect: 100-contimue' headers for PUT
 */
 
@@ -184,7 +184,15 @@ public class B6726695 extends Thread {
         out.flush();
         // Then read the body
         char[] cbuf = new char[512];
-        int l = in.read(cbuf);
+        in.read(cbuf);
+
+        /* Force the server to not respond for more that the expect 100-Continue
+         * timeout set by the HTTP handler (5000 millis). This ensures the
+         * timeout is correctly resets the default read timeout, infinity.
+         * See 6993490. */
+        System.out.println("server sleeping...");
+        try {Thread.sleep(6000); } catch (InterruptedException e) {}
+
         // finally send the 200 OK
         out.print("HTTP/1.1 200 OK");
         out.print("Server: Sun-Java-System-Web-Server/7.0\r\n");

test/sun/net/www/http/KeepAliveCache/B5045306.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 5045306 6356004
+ * @bug 5045306 6356004 6993490
  * @library ../../httptest/
  * @build HttpCallback HttpServer HttpTransaction
  * @run main/othervm B5045306
@@ -32,7 +32,6 @@
 
 import java.net.*;
 import java.io.*;
-import java.nio.channels.*;
 import java.lang.management.*;
 
 /* Part 1:
@@ -164,6 +163,14 @@ class SimpleHttpTransaction implements HttpCallback
                     failed = true;
 
                 trans.setResponseHeader ("Content-length", Integer.toString(0));
+
+                 /* Force the server to not respond for more that the timeout
+                  * set by the keepalive cleaner (5000 millis). This ensures the
+                  * timeout is correctly resets the default read timeout,
+                  * infinity. See 6993490. */
+                System.out.println("server sleeping...");
+                try {Thread.sleep(6000); } catch (InterruptedException e) {}
+
                 trans.sendResponse(200, "OK");
             } else if(path.equals("/part2")) {
                 System.out.println("Call to /part2");

test/sun/net/www/protocol/http/ChunkedErrorStream.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 6488669 6595324
+ * @bug 6488669 6595324 6993490
  * @run main/othervm ChunkedErrorStream
  * @summary Chunked ErrorStream tests
  */
@@ -48,6 +48,18 @@ import com.sun.net.httpserver.*;
  * 2) Client sends request to server and tries to
  *    getErrorStream(). 4K + 10 bytes must be read from
  *    the errorStream.
+ *
+ * Part 3: 6993490
+ *    Reuse persistent connection from part 2, the error stream
+ *    buffering will have set a reduced timeout on the socket and
+ *    tried to reset it to the default, infinity. Client must not
+ *    throw a timeout exception. If it does, it indicates that the
+ *    default timeout was not reset correctly.
+ *    If no timeout exception is thrown, it does not guarantee that
+ *    the timeout was reset correctly, as there is a potential race
+ *    between the sleeping server and the client thread. Typically,
+ *    1000 millis has been enought to reliable reproduce this problem
+ *    since the error stream buffering sets the timeout to 60 millis.
  */
 
 public class ChunkedErrorStream
@@ -75,19 +87,18 @@ public class ChunkedErrorStream
         }  finally {
             httpServer.stop(1);
         }
-
     }
 
     void doClient() {
-        for (int times=0; times<2; times++) {
+        for (int times=0; times<3; times++) {
             HttpURLConnection uc = null;
             try {
                 InetSocketAddress address = httpServer.getAddress();
                 String URLStr = "http://localhost:" + address.getPort() + "/test/";
                 if (times == 0) {
-                    URLStr += 6488669;
+                    URLStr += "first";
                 } else {
-                    URLStr += 6595324;
+                    URLStr += "second";
                 }
 
                 System.out.println("Trying " + URLStr);
@@ -97,6 +108,11 @@ public class ChunkedErrorStream
 
                 throw new RuntimeException("Failed: getInputStream should throw and IOException");
             }  catch (IOException e) {
+                if (e instanceof SocketTimeoutException) {
+                    e.printStackTrace();
+                    throw new RuntimeException("Failed: SocketTimeoutException should not happen");
+                }
+
                 // This is what we expect to happen.
                 InputStream es = uc.getErrorStream();
                 byte[] ba = new byte[1024];
@@ -112,7 +128,7 @@ public class ChunkedErrorStream
                 if (count == 0)
                     throw new RuntimeException("Failed: ErrorStream returning 0 bytes");
 
-                if (times == 1 && count != (4096+10))
+                if (times >= 1 && count != (4096+10))
                     throw new RuntimeException("Failed: ErrorStream returning " + count +
                                                  " bytes. Expecting " + (4096+10));
 
@@ -128,13 +144,13 @@ public class ChunkedErrorStream
         httpServer = com.sun.net.httpserver.HttpServer.create(new InetSocketAddress(0), 0);
 
         // create HttpServer context
-        HttpContext ctx1 = httpServer.createContext("/test/6488669", new Handler6488669());
-        HttpContext ctx2 = httpServer.createContext("/test/6595324", new Handler6595324());
+        httpServer.createContext("/test/first", new FirstHandler());
+        httpServer.createContext("/test/second", new SecondHandler());
 
         httpServer.start();
     }
 
-    class Handler6488669 implements HttpHandler {
+    class FirstHandler implements HttpHandler {
         public void handle(HttpExchange t) throws IOException {
             InputStream is = t.getRequestBody();
             byte[] ba = new byte[1024];
@@ -156,13 +172,22 @@ public class ChunkedErrorStream
         }
     }
 
-    class Handler6595324 implements HttpHandler {
+    static class SecondHandler implements HttpHandler {
+        /* count greater than 0, slow response */
+        static int count = 0;
+
         public void handle(HttpExchange t) throws IOException {
             InputStream is = t.getRequestBody();
             byte[] ba = new byte[1024];
             while (is.read(ba) != -1);
             is.close();
 
+            if (count > 0) {
+                System.out.println("server sleeping...");
+                try { Thread.sleep(1000); } catch(InterruptedException e) {}
+            }
+            count++;
+
             t.sendResponseHeaders(404, 0);
             OutputStream os = t.getResponseBody();