Don't retry unsafe HTTP methods in case of an IOException

If an IOException is thrown during the execution of an HttpCommand retry only if the HTTP method is idempotent (i.e. GET, DELETE, PUT). Otherwise the retry could cause unwanted side effects (i.e. creating and leaking multiple new nodes).

core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java

@@ -24,6 +24,7 @@ import static org.jclouds.http.HttpUtils.wirePayloadIfEnabled;
 import static org.jclouds.util.Throwables2.getFirstThrowableOfType;
 
 import java.io.IOException;
+import java.util.Set;
 
 import javax.annotation.Resource;
 import javax.inject.Inject;
@@ -44,8 +45,11 @@ import org.jclouds.io.ContentMetadataCodec;
 import org.jclouds.logging.Logger;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
 
 public abstract class BaseHttpCommandExecutorService<Q> implements HttpCommandExecutorService {
+   private static final Set<String> IDEMPOTENT_METHODS = ImmutableSet.of("GET", "HEAD", "OPTIONS", "PUT", "DELETE");
+
    protected final HttpUtils utils;
    protected final ContentMetadataCodec contentMetadataCodec;
 
@@ -107,7 +111,7 @@ public abstract class BaseHttpCommandExecutorService<Q> implements HttpCommandEx
             }
          } catch (Exception e) {
             IOException ioe = getFirstThrowableOfType(e, IOException.class);
-            if (ioe != null && ioRetryHandler.shouldRetryRequest(command, ioe)) {
+            if (ioe != null && shouldContinue(command, ioe)) {
                continue;
             }
             command.setException(new HttpResponseException(e.getMessage() + " connecting to "
@@ -137,6 +141,20 @@ public abstract class BaseHttpCommandExecutorService<Q> implements HttpCommandEx
       return shouldContinue;
    }
 
+   boolean shouldContinue(HttpCommand command, IOException response) {
+      return isIdempotent(command) && ioRetryHandler.shouldRetryRequest(command, response);
+   }
+
+   private boolean isIdempotent(HttpCommand command) {
+      String method = command.getCurrentRequest().getMethod();
+      if (!IDEMPOTENT_METHODS.contains(method)) {
+         logger.error("Command not considered safe to retry because request method is %1$s: %2$s", method, command);
+         return false;
+      } else {
+         return true;
+      }
+   }
+
    protected abstract Q convert(HttpRequest request) throws IOException, InterruptedException;
 
    protected abstract HttpResponse invoke(Q nativeRequest) throws IOException, InterruptedException;

core/src/test/java/org/jclouds/http/internal/BaseHttpCommandExecutorServiceTest.java

@@ -27,6 +27,7 @@ import static org.jclouds.io.Payloads.newInputStreamPayload;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -36,7 +37,9 @@ import javax.inject.Inject;
 import org.easymock.EasyMock;
 import org.easymock.IAnswer;
 import org.jclouds.http.HttpCommand;
+import org.jclouds.http.HttpException;
 import org.jclouds.http.HttpRequest;
+import org.jclouds.http.HttpRequestFilter;
 import org.jclouds.http.HttpResponse;
 import org.jclouds.http.HttpUtils;
 import org.jclouds.http.IOExceptionRetryHandler;
@@ -196,6 +199,41 @@ public class BaseHttpCommandExecutorServiceTest {
       assertEquals(response.getPayload().openStream().read(), -1);
    }
 
+   public void testDoNotRetryPostOnException() throws IOException {
+      helperRetryOnlyIdempotent("POST");
+   }
+
+   public void testRetryGetOnException() throws IOException {
+      helperRetryOnlyIdempotent("GET");
+   }
+
+   private void helperRetryOnlyIdempotent(String method) throws IOException {
+      final IOException error = new IOException("test exception");
+      HttpRequestFilter throwingFilter = new HttpRequestFilter() {
+         @Override
+         public HttpRequest filter(HttpRequest request) throws HttpException {
+            throw new HttpException(error);
+         }
+      };
+      HttpCommand command = new HttpCommand(HttpRequest.builder().endpoint("http://localhost").method(method).filter(throwingFilter).build());
+
+      IOExceptionRetryHandler ioRetryHandler = EasyMock.createMock(IOExceptionRetryHandler.class);
+
+      if ("GET".equals(method)) {
+         expect(ioRetryHandler.shouldRetryRequest(command, error)).andReturn(true);
+         expect(ioRetryHandler.shouldRetryRequest(command, error)).andReturn(false);
+      }
+      replay(ioRetryHandler);
+
+      BaseHttpCommandExecutorService<?> service = mockHttpCommandExecutorService(ioRetryHandler);
+      try {
+         service.invoke(command);
+         fail("Expected to fail due to throwing filter");
+      } catch (Exception e) {}
+
+      verify(ioRetryHandler);
+   }
+
    private HttpCommand mockHttpCommand() {
       return new HttpCommand(HttpRequest.builder().endpoint("http://localhost").method("mock").build());
    }
@@ -215,6 +253,19 @@ public class BaseHttpCommandExecutorServiceTest {
       return injector.getInstance(BaseHttpCommandExecutorService.class);
    }
 
+   private BaseHttpCommandExecutorService<?> mockHttpCommandExecutorService(final IOExceptionRetryHandler ioRetryHandler) {
+      Injector injector = Guice.createInjector(new AbstractModule() {
+         @Override
+         protected void configure() {
+            Names.bindProperties(binder(), BaseHttpApiMetadata.defaultProperties());
+            bind(IOExceptionRetryHandler.class).toInstance(ioRetryHandler);
+            bind(BaseHttpCommandExecutorService.class).to(MockHttpCommandExecutorService.class);
+         }
+      });
+
+      return injector.getInstance(BaseHttpCommandExecutorService.class);
+   }
+
    private static class MockInputStream extends InputStream {
       boolean isOpen = true;
       int count;