From 41ff84bf780a7ac582fdb7c31824b003e21795d5 Mon Sep 17 00:00:00 2001 From: Svetoslav Neykov Date: Wed, 13 May 2015 16:09:44 +0300 Subject: [PATCH] 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). --- .../BaseHttpCommandExecutorService.java | 20 +++++++- .../BaseHttpCommandExecutorServiceTest.java | 51 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java b/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java index e77065877f..dd2f03d262 100644 --- a/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java +++ b/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 implements HttpCommandExecutorService { + private static final Set 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 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 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; diff --git a/core/src/test/java/org/jclouds/http/internal/BaseHttpCommandExecutorServiceTest.java b/core/src/test/java/org/jclouds/http/internal/BaseHttpCommandExecutorServiceTest.java index 8fa399ec24..ee951a5018 100644 --- a/core/src/test/java/org/jclouds/http/internal/BaseHttpCommandExecutorServiceTest.java +++ b/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;