Merge pull request Azure#98 from emgerner-msft/retryfix

Azure#94 fixed 304 retry
g2vinay · May 31, 2016 · 54fcb9c · 54fcb9c
2 parents 3a469fc + 7428f7d
commit 54fcb9c
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 6 deletions.
diff --git a/ChangeLog.txt b/ChangeLog.txt
@@ -1,5 +1,6 @@
 2016.x.x Version 4.3.0
  * Added support for getBlobReferenceFromServer methods on CloudBlobContainer to support retrieving a blob without knowing its type.
+ * Fixed a bug in the retry policies where 300 status codes were being retried when they shouldn't be.
 
 2016.04.07 Version 4.2.0
  * Added support for setting a library-wide proxy. The default proxy can be set on OperationContext.

diff --git a/microsoft-azure-storage-test/src/com/microsoft/azure/storage/SecondaryTests.java b/microsoft-azure-storage-test/src/com/microsoft/azure/storage/SecondaryTests.java
@@ -16,6 +16,8 @@
 
 import static org.junit.Assert.*;
 
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
@@ -30,6 +32,7 @@
 import com.microsoft.azure.storage.TestRunners.SlowTests;
 import com.microsoft.azure.storage.blob.BlobRequestOptions;
 import com.microsoft.azure.storage.blob.BlobTestHelper;
+import com.microsoft.azure.storage.blob.BlobType;
 import com.microsoft.azure.storage.blob.CloudBlobClient;
 import com.microsoft.azure.storage.blob.CloudBlobContainer;
 import com.microsoft.azure.storage.blob.CloudBlockBlob;
@@ -639,6 +642,32 @@ public void testMultiLocationRetriesTable() throws URISyntaxException, StorageEx
         testTableDownloadPermissions(null, LocationMode.SECONDARY_THEN_PRIMARY, StorageLocation.SECONDARY,
                 retryContextList, retryInfoList);
     }
+
+    @Test
+    public void testRetryOn304() throws StorageException, IOException, URISyntaxException {
+        OperationContext operationContext = new OperationContext();
+        operationContext.getRetryingEventHandler().addListener(new StorageEvent<RetryingEvent>() {
+            @Override
+            public void eventOccurred(RetryingEvent eventArg) {
+                fail("Request should not be retried.");
+            }
+        });
+
+        CloudBlobContainer container = BlobTestHelper.getRandomContainerReference();
+        try {
+            container.create();
+            CloudBlockBlob blockBlobRef = (CloudBlockBlob) BlobTestHelper.uploadNewBlob(container, BlobType.BLOCK_BLOB,
+                    "originalBlob", 1024, null);
+            AccessCondition accessCondition = AccessCondition.generateIfNoneMatchCondition(blockBlobRef.getProperties().getEtag());
+            blockBlobRef.download(new ByteArrayOutputStream(), accessCondition, null, operationContext);
+
+            fail("Download should fail with a 304.");
+        } catch (StorageException ex) {
+            assertEquals("The condition specified using HTTP conditional header(s) is not met.", ex.getMessage());
+        } finally {
+            container.deleteIfExists();
+        }
+    }
 
     private static void AddUpdatedLocationModes(List<RetryContext> retryContextList, List<RetryInfo> retryInfoList) {
         retryInfoList

diff --git a/microsoft-azure-storage/src/com/microsoft/azure/storage/RetryExponentialRetry.java b/microsoft-azure-storage/src/com/microsoft/azure/storage/RetryExponentialRetry.java
@@ -115,11 +115,14 @@ public RetryInfo evaluate(RetryContext retryContext, OperationContext operationC
         boolean secondaryNotFound = this.evaluateLastAttemptAndSecondaryNotFound(retryContext);
 
         if (retryContext.getCurrentRetryCount() < this.maximumAttempts) {
+
+            // If this method is called after a successful response, it means
+            // we failed during the response body download. So, we should not
+            // check for success codes here. 
             int statusCode = retryContext.getLastRequestResult().getStatusCode();
-            if ((!secondaryNotFound && statusCode >= 400 && statusCode < 500)
+            if ((!secondaryNotFound && statusCode >= 300 && statusCode < 500 && statusCode != 408)
                     || statusCode == HttpURLConnection.HTTP_NOT_IMPLEMENTED
-                    || statusCode == HttpURLConnection.HTTP_VERSION
-                    || statusCode == Constants.HeaderConstants.HTTP_UNUSED_306) {
+                    || statusCode == HttpURLConnection.HTTP_VERSION) {
                 return null;
             }
 

diff --git a/microsoft-azure-storage/src/com/microsoft/azure/storage/RetryLinearRetry.java b/microsoft-azure-storage/src/com/microsoft/azure/storage/RetryLinearRetry.java
@@ -77,11 +77,14 @@ public RetryInfo evaluate(RetryContext retryContext, OperationContext operationC
         boolean secondaryNotFound = this.evaluateLastAttemptAndSecondaryNotFound(retryContext);
 
         if (retryContext.getCurrentRetryCount() < this.maximumAttempts) {
+
+            // If this method is called after a successful response, it means
+            // we failed during the response body download. So, we should not
+            // check for success codes here.            
             int statusCode = retryContext.getLastRequestResult().getStatusCode();
-            if ((!secondaryNotFound && statusCode >= 400 && statusCode < 500)
+            if ((!secondaryNotFound && statusCode >= 300 && statusCode < 500 && statusCode != 408)
                     || statusCode == HttpURLConnection.HTTP_NOT_IMPLEMENTED
-                    || statusCode == HttpURLConnection.HTTP_VERSION
-                    || statusCode == Constants.HeaderConstants.HTTP_UNUSED_306) {
+                    || statusCode == HttpURLConnection.HTTP_VERSION) {
                 return null;
             }