From 05a3db3cc085c21a6815a215affaf2aa20509aa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=20D=C3=B6rnbrack?= Date: Mon, 25 Sep 2017 19:59:31 +0200 Subject: [PATCH] Fix zuul post retry (#2209) Modfied RibbonCommandContext to make request entity resettable. fixes gh-892 --- .../main/asciidoc/spring-cloud-netflix.adoc | 3 + .../filters/route/RibbonCommandContext.java | 69 +++++++++------- .../ResettableServletInputStreamWrapper.java | 53 +++++++++++++ .../route/RibbonCommandContextTest.java | 78 +++++++++++++++++++ 4 files changed, 177 insertions(+), 26 deletions(-) create mode 100644 spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/support/ResettableServletInputStreamWrapper.java create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContextTest.java diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc index 9ff84004..9ccddcaa 100644 --- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc +++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc @@ -2685,6 +2685,9 @@ certain Ribbon properties. The properties you can use are `client.ribbon.OkToRetryOnAllOperations`. See the https://github.com/Netflix/ribbon/wiki/Getting-Started#the-properties-file-sample-clientproperties[Ribbon documentation] for a description of what there properties do. +WARNING: Enabling `client.ribbon.OkToRetryOnAllOperations` includes retring POST requests wich can have a impact +on the server's resources due to the buffering of the request's body. + In addition you may want to retry requests when certain status codes are returned in the response. You can list the response codes you would like the Ribbon client to retry using the property `clientName.ribbon.retryableStatusCodes`. For example diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContext.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContext.java index e5c6e343..4e77e1a7 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContext.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContext.java @@ -16,6 +16,13 @@ package org.springframework.cloud.netflix.zuul.filters.route; +import org.springframework.cloud.netflix.ribbon.support.RibbonRequestCustomizer; +import org.springframework.cloud.netflix.zuul.filters.support.ResettableServletInputStreamWrapper; +import org.springframework.util.Assert; +import org.springframework.util.MultiValueMap; +import org.springframework.util.ReflectionUtils; +import org.springframework.util.StreamUtils; + import java.io.InputStream; import java.net.URI; import java.net.URISyntaxException; @@ -23,11 +30,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Objects; -import org.springframework.cloud.netflix.ribbon.support.RibbonRequestCustomizer; -import org.springframework.util.Assert; -import org.springframework.util.MultiValueMap; -import org.springframework.util.ReflectionUtils; - /** * @author Spencer Gibb */ @@ -38,26 +40,27 @@ public class RibbonCommandContext { private final Boolean retryable; private final MultiValueMap headers; private final MultiValueMap params; - private final InputStream requestEntity; private final List requestCustomizers; + private InputStream requestEntity; private Long contentLength; /** * Kept for backwards compatibility with Spring Cloud Sleuth 1.x versions */ @Deprecated - public RibbonCommandContext(String serviceId, String method, String uri, - Boolean retryable, MultiValueMap headers, - MultiValueMap params, InputStream requestEntity) { + public RibbonCommandContext(String serviceId, String method, + String uri, Boolean retryable, MultiValueMap headers, + MultiValueMap params, InputStream requestEntity) { this(serviceId, method, uri, retryable, headers, params, requestEntity, - new ArrayList(), null); + new ArrayList(), null); } public RibbonCommandContext(String serviceId, String method, String uri, Boolean retryable, MultiValueMap headers, MultiValueMap params, InputStream requestEntity, List requestCustomizers) { - this(serviceId, method, uri, retryable, headers, params, requestEntity, requestCustomizers, null); + this(serviceId, method, uri, retryable, headers, params, requestEntity, + requestCustomizers, null); } public RibbonCommandContext(String serviceId, String method, String uri, @@ -84,8 +87,7 @@ public class RibbonCommandContext { public URI uri() { try { return new URI(this.uri); - } - catch (URISyntaxException e) { + } catch (URISyntaxException e) { ReflectionUtils.rethrowRuntimeException(e); } return null; @@ -93,6 +95,7 @@ public class RibbonCommandContext { /** * Use getMethod() + * * @return */ @Deprecated @@ -125,7 +128,19 @@ public class RibbonCommandContext { } public InputStream getRequestEntity() { - return requestEntity; + if (requestEntity == null) { + return requestEntity; + } + + try { + if (!(requestEntity instanceof ResettableServletInputStreamWrapper)) { + requestEntity = new ResettableServletInputStreamWrapper( + StreamUtils.copyToByteArray(requestEntity)); + } + requestEntity.reset(); + } finally { + return requestEntity; + } } public List getRequestCustomizers() { @@ -142,23 +157,25 @@ public class RibbonCommandContext { @Override public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; RibbonCommandContext that = (RibbonCommandContext) o; - return Objects.equals(serviceId, that.serviceId) && - Objects.equals(method, that.method) && - Objects.equals(uri, that.uri) && - Objects.equals(retryable, that.retryable) && - Objects.equals(headers, that.headers) && - Objects.equals(params, that.params) && - Objects.equals(requestEntity, that.requestEntity) && - Objects.equals(requestCustomizers, that.requestCustomizers) && - Objects.equals(contentLength, that.contentLength); + return Objects.equals(serviceId, that.serviceId) && Objects + .equals(method, that.method) && Objects.equals(uri, that.uri) + && Objects.equals(retryable, that.retryable) && Objects + .equals(headers, that.headers) && Objects + .equals(params, that.params) && Objects + .equals(requestEntity, that.requestEntity) && Objects + .equals(requestCustomizers, that.requestCustomizers) && Objects + .equals(contentLength, that.contentLength); } @Override public int hashCode() { - return Objects.hash(serviceId, method, uri, retryable, headers, params, requestEntity, requestCustomizers, contentLength); + return Objects.hash(serviceId, method, uri, retryable, headers, params, + requestEntity, requestCustomizers, contentLength); } @Override diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/support/ResettableServletInputStreamWrapper.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/support/ResettableServletInputStreamWrapper.java new file mode 100644 index 00000000..e44d8bcb --- /dev/null +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/support/ResettableServletInputStreamWrapper.java @@ -0,0 +1,53 @@ +/* + * Copyright 2013-2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.cloud.netflix.zuul.filters.support; + +import javax.servlet.ReadListener; +import javax.servlet.ServletInputStream; +import java.io.ByteArrayInputStream; +import java.io.IOException; + +public class ResettableServletInputStreamWrapper extends ServletInputStream { + private final ByteArrayInputStream input; + + public ResettableServletInputStreamWrapper(byte[] data) { + this.input = new ByteArrayInputStream(data); + } + + @Override + public boolean isFinished() { + return false; + } + + @Override + public boolean isReady() { + return false; + } + + @Override + public void setReadListener(ReadListener listener) { + } + + @Override + public int read() throws IOException { + return input.read(); + } + + @Override + public synchronized void reset() throws IOException { + input.reset(); + } +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContextTest.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContextTest.java new file mode 100644 index 00000000..1c0026f0 --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContextTest.java @@ -0,0 +1,78 @@ +package org.springframework.cloud.netflix.zuul.filters.route; + +import com.google.common.collect.Lists; +import okhttp3.Request; +import org.junit.Test; +import org.springframework.cloud.netflix.ribbon.support.RibbonRequestCustomizer; +import org.springframework.cloud.netflix.zuul.filters.support.ResettableServletInputStreamWrapper; +import org.springframework.http.HttpMethod; +import org.springframework.util.LinkedMultiValueMap; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +/** + * @author Andre Dörnbrack + */ +public class RibbonCommandContextTest { + + private static final byte[] TEST_CONTENT = { 42, 42, 42, 42, 42 }; + + private RibbonCommandContext ribbonCommandContext; + + @Test + public void testMultipleReadsOnRequestEntity() throws Exception { + givenRibbonCommandContextIsSetup(); + + InputStream requestEntity = ribbonCommandContext.getRequestEntity(); + assertTrue(requestEntity instanceof ResettableServletInputStreamWrapper); + + whenInputStreamIsConsumed(requestEntity); + assertEquals(-1, requestEntity.read()); + + requestEntity.reset(); + assertNotEquals(-1, requestEntity.read()); + + whenInputStreamIsConsumed(requestEntity); + assertEquals(-1, requestEntity.read()); + + requestEntity.reset(); + assertNotEquals(-1, requestEntity.read()); + + whenInputStreamIsConsumed(requestEntity); + assertEquals(-1, requestEntity.read()); + } + + private void whenInputStreamIsConsumed(InputStream requestEntity) throws IOException { + while (requestEntity.read() != -1) { + requestEntity.read(); + } + } + + private void givenRibbonCommandContextIsSetup() { + LinkedMultiValueMap headers = new LinkedMultiValueMap(); + LinkedMultiValueMap params = new LinkedMultiValueMap(); + + RibbonRequestCustomizer requestCustomizer = new RibbonRequestCustomizer() { + @Override + public boolean accepts(Class builderClass) { + return builderClass == Request.Builder.class; + } + + @Override + public void customize(Request.Builder builder) { + builder.addHeader("from-customizer", "foo"); + } + }; + + ribbonCommandContext = new RibbonCommandContext("serviceId", + HttpMethod.POST.toString(), "/my/route", true, headers, params, + new ByteArrayInputStream(TEST_CONTENT), + Lists.newArrayList(requestCustomizer)); + } +} \ No newline at end of file