Skip to content

Commit e02c106

Browse files
ok2cok2c
committed
HTTPCLIENT-2141: revision of the original implementation / better debug logging / additional integration tests
1 parent 7000b9e commit e02c106

9 files changed

Lines changed: 307 additions & 93 deletions

File tree

‎httpclient5-testing/src/main/java/org/apache/hc/client5/testing/classic/ServiceUnavailableDecorator.java‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/*
2+
* ====================================================================
3+
* Licensed to the Apache Software Foundation (ASF) under one
4+
* or more contributor license agreements. See the NOTICE file
5+
* distributed with this work for additional information
6+
* regarding copyright ownership. The ASF licenses this file
7+
* to you under the Apache License, Version 2.0 (the
8+
* "License"); you may not use this file except in compliance
9+
* with the License. You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing,
14+
* software distributed under the License is distributed on an
15+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
16+
* KIND, either express or implied. See the License for the
17+
* specific language governing permissions and limitations
18+
* under the License.
19+
* ====================================================================
20+
*
21+
* This software consists of voluntary contributions made by many
22+
* individuals on behalf of the Apache Software Foundation. For more
23+
* information on the Apache Software Foundation, please see
24+
* <http://www.apache.org/>.
25+
*
26+
*/
27+
28+
package org.apache.hc.client5.testing.classic;
29+
30+
import java.io.IOException;
31+
import java.util.concurrent.atomic.AtomicBoolean;
32+
33+
import org.apache.hc.core5.function.Resolver;
34+
import org.apache.hc.core5.http.ClassicHttpRequest;
35+
import org.apache.hc.core5.http.ClassicHttpResponse;
36+
import org.apache.hc.core5.http.HttpException;
37+
import org.apache.hc.core5.http.HttpHeaders;
38+
import org.apache.hc.core5.http.HttpRequest;
39+
import org.apache.hc.core5.http.HttpStatus;
40+
import org.apache.hc.core5.http.HttpVersion;
41+
import org.apache.hc.core5.http.ProtocolVersion;
42+
import org.apache.hc.core5.http.io.HttpServerRequestHandler;
43+
import org.apache.hc.core5.http.message.BasicClassicHttpResponse;
44+
import org.apache.hc.core5.http.protocol.HttpContext;
45+
import org.apache.hc.core5.util.Args;
46+
import org.apache.hc.core5.util.TimeValue;
47+
48+
public class ServiceUnavailableDecorator implements HttpServerRequestHandler {
49+
50+
private final HttpServerRequestHandler requestHandler;
51+
private final Resolver<HttpRequest, TimeValue> serviceAvailabilityResolver;
52+
private final AtomicBoolean serviceUnavailable;
53+
54+
public ServiceUnavailableDecorator(final HttpServerRequestHandler requestHandler,
55+
final Resolver<HttpRequest, TimeValue> serviceAvailabilityResolver) {
56+
this.requestHandler = Args.notNull(requestHandler, "Request handler");
57+
this.serviceAvailabilityResolver = Args.notNull(serviceAvailabilityResolver, "Service availability resolver");
58+
this.serviceUnavailable = new AtomicBoolean();
59+
}
60+
61+
@Override
62+
public void handle(final ClassicHttpRequest request,
63+
final ResponseTrigger responseTrigger,
64+
final HttpContext context) throws HttpException, IOException {
65+
final TimeValue retryAfter = serviceAvailabilityResolver.resolve(request);
66+
serviceUnavailable.set(TimeValue.isPositive(retryAfter));
67+
if (serviceUnavailable.get()) {
68+
final ClassicHttpResponse response = new BasicClassicHttpResponse(HttpStatus.SC_SERVICE_UNAVAILABLE);
69+
response.addHeader(HttpHeaders.RETRY_AFTER, Long.toString(retryAfter.toSeconds()));
70+
final ProtocolVersion version = request.getVersion();
71+
if (version != null && version.compareToVersion(HttpVersion.HTTP_2) < 0) {
72+
response.addHeader(HttpHeaders.CONNECTION, "Close");
73+
}
74+
responseTrigger.submitResponse(response);
75+
} else {
76+
requestHandler.handle(request, responseTrigger, context);
77+
}
78+
}
79+
80+
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/HttpIntegrationTests.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,4 +112,24 @@ public RedirectsTls() {
112112

113113
}
114114

115+
@Nested
116+
@DisplayName("Request re-execution (HTTP/1.1)")
117+
class RequestReExecution extends TestClientRequestReExecution {
118+
119+
public RequestReExecution() {
120+
super(URIScheme.HTTP);
121+
}
122+
123+
}
124+
125+
@Nested
126+
@DisplayName("Request re-execution (HTTP/1.1)")
127+
class RequestReExecutionTls extends TestClientRequestReExecution {
128+
129+
public RequestReExecutionTls() {
130+
super(URIScheme.HTTPS);
131+
}
132+
133+
}
134+
115135
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestReExecution.java‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
/*
2+
* ====================================================================
3+
* Licensed to the Apache Software Foundation (ASF) under one
4+
* or more contributor license agreements. See the NOTICE file
5+
* distributed with this work for additional information
6+
* regarding copyright ownership. The ASF licenses this file
7+
* to you under the Apache License, Version 2.0 (the
8+
* "License"); you may not use this file except in compliance
9+
* with the License. You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing,
14+
* software distributed under the License is distributed on an
15+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
16+
* KIND, either express or implied. See the License for the
17+
* specific language governing permissions and limitations
18+
* under the License.
19+
* ====================================================================
20+
*
21+
* This software consists of voluntary contributions made by many
22+
* individuals on behalf of the Apache Software Foundation. For more
23+
* information on the Apache Software Foundation, please see
24+
* <http://www.apache.org/>.
25+
*
26+
*/
27+
package org.apache.hc.client5.testing.sync;
28+
29+
import static org.hamcrest.MatcherAssert.assertThat;
30+
31+
import java.util.concurrent.atomic.AtomicInteger;
32+
import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy;
33+
import org.apache.hc.client5.http.protocol.HttpClientContext;
34+
import org.apache.hc.client5.testing.classic.RandomHandler;
35+
import org.apache.hc.client5.testing.classic.ServiceUnavailableDecorator;
36+
import org.apache.hc.client5.testing.extension.sync.ClientProtocolLevel;
37+
import org.apache.hc.client5.testing.extension.sync.TestClient;
38+
import org.apache.hc.core5.function.Resolver;
39+
import org.apache.hc.core5.http.ClassicHttpRequest;
40+
import org.apache.hc.core5.http.HttpHost;
41+
import org.apache.hc.core5.http.HttpRequest;
42+
import org.apache.hc.core5.http.HttpResponse;
43+
import org.apache.hc.core5.http.HttpStatus;
44+
import org.apache.hc.core5.http.URIScheme;
45+
import org.apache.hc.core5.http.io.entity.EntityUtils;
46+
import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
47+
import org.apache.hc.core5.util.TimeValue;
48+
import org.hamcrest.CoreMatchers;
49+
import org.junit.jupiter.api.BeforeEach;
50+
import org.junit.jupiter.api.Test;
51+
52+
abstract class TestClientRequestReExecution extends AbstractIntegrationTestBase {
53+
54+
public TestClientRequestReExecution(final URIScheme scheme) {
55+
super(scheme, ClientProtocolLevel.STANDARD);
56+
}
57+
58+
@BeforeEach
59+
void setup() {
60+
final Resolver<HttpRequest, TimeValue> serviceAvailabilityResolver = new Resolver<HttpRequest, TimeValue>() {
61+
62+
private final AtomicInteger count = new AtomicInteger(0);
63+
64+
@Override
65+
public TimeValue resolve(final HttpRequest request) {
66+
final int n = count.incrementAndGet();
67+
return n <= 3 ? TimeValue.ofSeconds(1) : null;
68+
}
69+
70+
};
71+
72+
configureServer(bootstrap -> bootstrap.setExchangeHandlerDecorator(handler
73+
-> new ServiceUnavailableDecorator(handler, serviceAvailabilityResolver)));
74+
}
75+
76+
@Test
77+
void testGiveUpAfterOneRetry() throws Exception {
78+
configureServer(bootstrap -> bootstrap.register("/random/*", new RandomHandler()));
79+
final HttpHost target = startServer();
80+
81+
configureClient(builder -> builder
82+
.setRetryStrategy(new DefaultHttpRequestRetryStrategy(1, TimeValue.ofSeconds(1))));
83+
final TestClient client = client();
84+
85+
final HttpClientContext context = HttpClientContext.create();
86+
final ClassicHttpRequest request = ClassicRequestBuilder.get()
87+
.setHttpHost(target)
88+
.setPath("/random/2048")
89+
.build();
90+
final HttpResponse response = client.execute(target, request, context, r -> {
91+
EntityUtils.consume(r.getEntity());
92+
return r;
93+
});
94+
assertThat(response.getCode(), CoreMatchers.equalTo(HttpStatus.SC_SERVICE_UNAVAILABLE));
95+
}
96+
97+
@Test
98+
void testDoNotGiveUpEasily() throws Exception {
99+
configureServer(bootstrap -> bootstrap.register("/random/*", new RandomHandler()));
100+
final HttpHost target = startServer();
101+
102+
configureClient(builder -> builder
103+
.setRetryStrategy(new DefaultHttpRequestRetryStrategy(5, TimeValue.ofSeconds(1))));
104+
final TestClient client = client();
105+
106+
final HttpClientContext context = HttpClientContext.create();
107+
final ClassicHttpRequest request = ClassicRequestBuilder.get()
108+
.setHttpHost(target)
109+
.setPath("/random/2048")
110+
.build();
111+
final HttpResponse response = client.execute(target, request, context, r -> {
112+
EntityUtils.consume(r.getEntity());
113+
return r;
114+
});
115+
assertThat(response.getCode(), CoreMatchers.equalTo(HttpStatus.SC_OK));
116+
}
117+
118+
}

‎httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -531,6 +531,12 @@ public Builder setConnectTimeout(final long connectTimeout, final TimeUnit timeU
531531
* HTTP transports with message multiplexing.
532532
* </p>
533533
* <p>
534+
* Please note that response timeout is not a deadline. Its absolute value
535+
* can be exceeded, for example, in case of automatic request re-execution.
536+
* Please make sure the automatic request re-execution policy has been
537+
* configured appropriately.
538+
* </p>
539+
* <p>
534540
* Default: {@code null}
535541
* </p>
536542
*

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultHttpRequestRetryStrategy.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@
4141
import javax.net.ssl.SSLException;
4242

4343
import org.apache.hc.client5.http.HttpRequestRetryStrategy;
44+
import org.apache.hc.client5.http.config.RequestConfig;
45+
import org.apache.hc.client5.http.protocol.HttpClientContext;
4446
import org.apache.hc.client5.http.utils.DateUtils;
4547
import org.apache.hc.core5.annotation.Contract;
4648
import org.apache.hc.core5.annotation.ThreadingBehavior;
@@ -55,6 +57,7 @@
5557
import org.apache.hc.core5.http.protocol.HttpContext;
5658
import org.apache.hc.core5.util.Args;
5759
import org.apache.hc.core5.util.TimeValue;
60+
import org.apache.hc.core5.util.Timeout;
5861

5962
/**
6063
* Default implementation of the {@link HttpRequestRetryStrategy} interface.
@@ -95,7 +98,8 @@ protected DefaultHttpRequestRetryStrategy(
9598
final Collection<Class<? extends IOException>> clazzes,
9699
final Collection<Integer> codes) {
97100
Args.notNegative(maxRetries, "maxRetries");
98-
Args.notNegative(defaultRetryInterval.getDuration(), "defaultRetryInterval");
101+
Args.notNull(defaultRetryInterval, "defaultRetryInterval");
102+
Args.check(TimeValue.isNonNegative(defaultRetryInterval), "Default retry interval is negative");
99103
this.maxRetries = maxRetries;
100104
this.defaultRetryInterval = defaultRetryInterval;
101105
this.nonRetriableIOExceptionClasses = new HashSet<>(clazzes);
@@ -199,6 +203,14 @@ public boolean retryRequest(
199203
final HttpContext context) {
200204
Args.notNull(response, "response");
201205

206+
if (context != null) {
207+
final HttpClientContext clientContext = HttpClientContext.cast(context);
208+
final RequestConfig requestConfig = clientContext.getRequestConfigOrDefault();
209+
final Timeout responseTimeout = requestConfig.getResponseTimeout();
210+
if (responseTimeout != null && defaultRetryInterval.compareTo(responseTimeout) > 0) {
211+
return false;
212+
}
213+
}
202214
return execCount <= this.maxRetries && retriableCodes.contains(response.getCode());
203215
}
204216

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncHttpRequestRetryExec.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,6 @@ public AsyncDataConsumer handleResponse(
126126
state.retrying = retryStrategy.retryRequest(response, scope.execCount.get(), clientContext);
127127
if (state.retrying) {
128128
state.delay = retryStrategy.getRetryInterval(response, scope.execCount.get(), clientContext);
129-
if (LOG.isDebugEnabled()) {
130-
LOG.debug("{} retrying request in {}", exchangeId, state.delay);
131-
}
132129
return new DiscardingEntityConsumer<>();
133130
}
134131
return asyncExecCallback.handleResponse(response, entityDetails);
@@ -146,13 +143,17 @@ public void completed() {
146143
if (entityProducer != null) {
147144
entityProducer.releaseResources();
148145
}
146+
final TimeValue delay = TimeValue.isPositive(state.delay) ? state.delay : TimeValue.ZERO_MILLISECONDS;
147+
if (LOG.isDebugEnabled()) {
148+
LOG.debug("{} wait for {}", exchangeId, delay);
149+
}
149150
scope.scheduler.scheduleExecution(
150151
request,
151152
entityProducer,
152153
scope,
153154
(r, e, s, c) -> execute(r, e, s, chain, c),
154155
asyncExecCallback,
155-
state.delay);
156+
delay);
156157
} else {
157158
asyncExecCallback.completed();
158159
}
@@ -182,13 +183,17 @@ public void failed(final Exception cause) {
182183
state.retrying = true;
183184
final int execCount = scope.execCount.incrementAndGet();
184185
state.delay = retryStrategy.getRetryInterval(request, (IOException) cause, execCount - 1, clientContext);
186+
final TimeValue delay = TimeValue.isPositive(state.delay) ? state.delay : TimeValue.ZERO_MILLISECONDS;
187+
if (LOG.isDebugEnabled()) {
188+
LOG.debug("{} wait for {}", exchangeId, delay);
189+
}
185190
scope.scheduler.scheduleExecution(
186191
request,
187192
entityProducer,
188193
scope,
189194
(r, e, s, c) -> execute(r, e, s, chain, c),
190195
asyncExecCallback,
191-
state.delay);
196+
delay);
192197
return;
193198
}
194199
}

0 commit comments

Comments
 (0)