Merge pull request #525 from rickle-msft/httpsKeepAlive

Https keep alive

Author: rickle-msft

ChangeLog.txt

@@ -1,3 +1,6 @@
+XXXX.XX.XX Version X.X.X
+ * Added a workaround to a JDK bug that would ignore connection timeouts on retries, causing hangs in some scenarios. This requires defaulting setting https keep-alive on all sockets. It can be disabled via BlobRequestOptions.
+
 2019.12.06 Version 8.6.0
  * Added the skipDecode flag to the generate sas method on CloudBlob. This flag allows the customer to skip the url decode that happens by default on the string to sign right before signing. This resolves some problems with custom values for some of the query parameters when used with third party clients.
 

microsoft-azure-storage/src/com/microsoft/azure/storage/RequestOptions.java

@@ -56,6 +56,11 @@ public abstract class RequestOptions {
      */
     private Boolean requireEncryption;
 
+    /**
+     * A value to indicate whether we should disable socket keep-alive.
+     */
+    private Boolean disableHttpsSocketKeepAlive;
+
     /**
      * Creates an instance of the <code>RequestOptions</code> class.
      */
@@ -78,6 +83,7 @@ public RequestOptions(final RequestOptions other) {
             this.setMaximumExecutionTimeInMs(other.getMaximumExecutionTimeInMs());
             this.setOperationExpiryTimeInMs(other.getOperationExpiryTimeInMs());
             this.setRequireEncryption(other.requireEncryption());
+            this.setDisableHttpsSocketKeepAlive(other.disableHttpsSocketKeepAlive());
         }
     }
 
@@ -100,6 +106,10 @@ protected static void applyBaseDefaultsInternal(final RequestOptions modifiedOpt
         if (modifiedOptions.requireEncryption() == null) {
             modifiedOptions.setRequireEncryption(false);
         }
+
+        if (modifiedOptions.disableHttpsSocketKeepAlive() == null) {
+            modifiedOptions.setDisableHttpsSocketKeepAlive(false);
+        }
     }
 
     /**
@@ -132,6 +142,10 @@ protected static void populateRequestOptions(RequestOptions modifiedOptions,
             modifiedOptions.setOperationExpiryTimeInMs(new Date().getTime()
                     + modifiedOptions.getMaximumExecutionTimeInMs());
         }
+
+        if (modifiedOptions.disableHttpsSocketKeepAlive() == null) {
+            modifiedOptions.setDisableHttpsSocketKeepAlive(clientOptions.disableHttpsSocketKeepAlive());
+        }
     }
 
     /**
@@ -178,18 +192,29 @@ public final LocationMode getLocationMode() {
     public Integer getMaximumExecutionTimeInMs() {
         return this.maximumExecutionTimeInMs;
     }
-    
+
     /**
      * Gets a value to indicate whether all data written and read must be encrypted. Use <code>true</code> to
      * encrypt/decrypt data for transactions; otherwise, <code>false</code>. For more
      * information about require encryption defaults, see {@link #setRequireEncryption(Boolean)}.
-     * 
+     *
      * @return A value to indicate whether all data written and read must be encrypted.
      */
     public Boolean requireEncryption() {
         return this.requireEncryption;
     }
 
+    /**
+     * Gets a value to indicate whether https socket keep-alive should be disabled. Use <code>true</code> to disable
+     * keep-alive; otherwise, <code>false</code>. For more information about disableHttpsSocketKeepAlive defaults, see
+     * {@link ServiceClient#getDefaultRequestOptions()}
+     *
+     * @return A value to indicate whther https socket keep-alive should be disabled.
+     */
+    public Boolean disableHttpsSocketKeepAlive() {
+        return this.disableHttpsSocketKeepAlive;
+    }
+
     /**
      * RESERVED FOR INTERNAL USE.
      * 
@@ -283,8 +308,8 @@ public void setMaximumExecutionTimeInMs(Integer maximumExecutionTimeInMs) {
      * <p>
      * The default is set in the client and is by default false, indicating encryption is not required. You can change
      * the value on this request by setting this property. You can also change the value on the
-     * {@link ServiceClient#getDefaultRequestOptions()} object so that all subsequent requests made via the service
-     * client will use the appropriate value.
+     * {@link ServiceClient#getDefaultRequestOptions()} object so that all subsequent requests made via the
+     * service client will use the appropriate value.
      * 
      * @param requireEncryption
      *            A value to indicate whether all data written and read must be encrypted.
@@ -293,6 +318,28 @@ public void setRequireEncryption(Boolean requireEncryption) {
         this.requireEncryption = requireEncryption;
     }
 
+    /**
+     * Sets a value to indicate whether https socket keep-alive should be disabled. Use <code>true</code> to disable
+     * keep-alive; otherwise, <code>false</code>
+     * <p>
+     * The default is set in the client and is by default false, indicating that https socket keep-alive will be
+     * enabled. You can change the value on this request by setting this property. You can also change the value on
+     * on the {@link ServiceClient#getDefaultRequestOptions()} object so that all subsequent requests made via the
+     * service client will use the appropriate value.
+     * <p>
+     * Setting keep-alive on https sockets is to work around a bug in the JVM where connection timeouts are not honored
+     * on retried requests. In those cases, we use socket keep-alive as a fallback. Unfortunately, the timeout value
+     * must be taken from a JVM property rather than configured locally. Therefore, in rare cases the JVM has configured
+     * aggressively short keep-alive times, it may be beneficial to disable the use of keep-alives lest they interfere
+     * with long running data transfer operations.
+     *
+     * @param disableHttpsSocketKeepAlive
+     *           A value to indicate whether https socket keep-alive should be disabled.
+     */
+    public void setDisableHttpsSocketKeepAlive(Boolean disableHttpsSocketKeepAlive) {
+        this.disableHttpsSocketKeepAlive = disableHttpsSocketKeepAlive;
+    }
+
     /**
      * RESERVED FOR INTERNAL USE.
      * 

microsoft-azure-storage/src/com/microsoft/azure/storage/core/BaseRequest.java

@@ -27,12 +27,14 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
-import java.util.Set;
 
 import com.microsoft.azure.storage.*;
-import com.microsoft.azure.storage.blob.BlobBatchOperation;
-import com.microsoft.azure.storage.blob.BlobRequestOptions;
-import com.microsoft.azure.storage.blob.CloudBlobClient;
+import com.microsoft.azure.storage.Constants;
+import com.microsoft.azure.storage.OperationContext;
+import com.microsoft.azure.storage.RequestOptions;
+import com.microsoft.azure.storage.StorageException;
+
+import javax.net.ssl.HttpsURLConnection;
 
 import static com.microsoft.azure.storage.Constants.QueryConstants.PROPERTIES;
 
@@ -223,6 +225,14 @@ protected PasswordAuthentication getPasswordAuthentication() {
             retConnection = (HttpURLConnection) resourceUrl.openConnection();
         }
 
+        /*
+        If we are using https, check if we should enable socket keep-alive timeouts to work around JVM bug.
+         */
+        if (retConnection instanceof HttpsURLConnection && !options.disableHttpsSocketKeepAlive()) {
+            HttpsURLConnection httpsConnection = ((HttpsURLConnection) retConnection);
+            httpsConnection.setSSLSocketFactory(new KeepAliveSocketFactory(httpsConnection.getSSLSocketFactory()));
+        }
+
         /*
          * ReadTimeout must be explicitly set to avoid a bug in JDK 6. In certain cases, this bug causes an immediate 
          * read timeout exception to be thrown even if ReadTimeout is not set.

microsoft-azure-storage/src/com/microsoft/azure/storage/core/KeepAliveSocketFactory.java

@@ -0,0 +1,78 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.microsoft.azure.storage.core;
+
+import javax.net.ssl.SSLSocketFactory;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+
+/**
+ * RESERVED FOR INTERNAL USE.
+ *
+ * This type is used to help work around a bug in the JDK where connection timeouts are not honored on a retried
+ * request. In other words, if a customer set a timeout on an operation, this timeout is only ever respected on the
+ * first attempt at the request. Retries will cause a different underlying connection implementation to be loaded that
+ * will ignore the timeout parameter. Therefore, requests can potentially hang forever if the connection is broken
+ * after these retries.
+ *
+ * Enabling keep-alive timeouts acts as a fallback in these scenarios so that, even if the operation timeout is ignored,
+ * the socket will still eventually timeout and the request will be cancelled. We enable keep alive timeouts via a
+ * wrapper implementation of a SocketFactory. We use a default socket factory to get sockets from the system and then
+ * simply set the keep-alive option to true before returning to the client. This factory will be set on the
+ * HttpsUrlConnection objects.
+ */
+public class KeepAliveSocketFactory extends SSLSocketFactory {
+    private SSLSocketFactory delegate;
+
+    KeepAliveSocketFactory(SSLSocketFactory delegate) {
+        this.delegate = delegate;
+    }
+
+    @Override
+    public String[] getDefaultCipherSuites() {
+        return delegate.getDefaultCipherSuites();
+    }
+
+    @Override
+    public String[] getSupportedCipherSuites() {
+        return delegate.getSupportedCipherSuites();
+    }
+
+    @Override
+    public Socket createSocket(Socket socket, String s, int i, boolean b) throws IOException {
+        Socket ret = delegate.createSocket(socket, s, i, b);
+        ret.setKeepAlive(true);
+        return ret;
+    }
+
+    @Override
+    public Socket createSocket(String s, int i) throws IOException, UnknownHostException {
+        Socket ret = delegate.createSocket(s, i);
+        ret.setKeepAlive(true);
+        return ret;
+    }
+
+    @Override
+    public Socket createSocket(String s, int i, InetAddress inetAddress, int i1) throws IOException, UnknownHostException {
+        Socket ret = delegate.createSocket(s, i, inetAddress, i1);
+        ret.setKeepAlive(true);
+        return ret;
+    }
+
+    @Override
+    public Socket createSocket(InetAddress inetAddress, int i) throws IOException {
+        Socket ret = delegate.createSocket(inetAddress, i);
+        ret.setKeepAlive(true);
+        return ret;
+    }
+
+    @Override
+    public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddress1, int i1) throws IOException {
+        Socket ret = delegate.createSocket(inetAddress, i, inetAddress1, i1);
+        ret.setKeepAlive(true);
+        return ret;
+    }
+}