Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] Remove retrying on timeout exception #13224

Merged
merged 3 commits into from
Dec 4, 2023
Merged

[java] Remove retrying on timeout exception #13224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce684d4
[java] Remove retrying on timeout exception
pujagani Nov 30, 2023
5593ab2
[java] Fix formatting
pujagani Dec 1, 2023
842bbb0
Merge branch 'trunk' into remove-retry
pujagani Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions java/src/org/openqa/selenium/remote/http/RetryRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import static com.google.common.net.HttpHeaders.CONTENT_LENGTH;
import static java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT;
import static java.net.HttpURLConnection.HTTP_GATEWAY_TIMEOUT;
import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
import static java.net.HttpURLConnection.HTTP_UNAVAILABLE;
import static org.openqa.selenium.internal.Debug.getDebugLogLevel;
Expand All @@ -33,7 +32,6 @@
import dev.failsafe.function.CheckedFunction;
import java.net.ConnectException;
import java.util.logging.Logger;
import org.openqa.selenium.TimeoutException;

public class RetryRequest implements Filter {

Expand All @@ -57,15 +55,6 @@ public class RetryRequest implements Filter {
e.getAttemptCount()))
.build();

// Retry on read timeout.
private static final RetryPolicy<HttpResponse> readTimeoutPolicy =
RetryPolicy.<HttpResponse>builder()
.handle(TimeoutException.class)
.withMaxRetries(3)
.onRetry(
e -> LOG.log(getDebugLogLevel(), "Read timeout #{0}. Retrying.", e.getAttemptCount()))
.build();

// Retry if server is unavailable or an internal server error occurs without response body.
private static final RetryPolicy<HttpResponse> serverErrorPolicy =
RetryPolicy.<HttpResponse>builder()
Expand All @@ -88,7 +77,6 @@ public HttpHandler apply(HttpHandler next) {
return req ->
Failsafe.with(fallback)
.compose(serverErrorPolicy)
.compose(readTimeoutPolicy)
.compose(connectionFailurePolicy)
.get(() -> next.execute(req));
}
Expand All @@ -102,11 +90,6 @@ private static HttpResponse getFallback(
.setStatus(HTTP_CLIENT_TIMEOUT)
.setContent(
asJson(ImmutableMap.of("value", ImmutableMap.of("message", "Connection failure"))));
} else if (exception instanceof TimeoutException) {
return new HttpResponse()
.setStatus(HTTP_GATEWAY_TIMEOUT)
.setContent(
asJson(ImmutableMap.of("value", ImmutableMap.of("message", "Read timeout"))));
} else throw exception;
} else if (executionAttemptedEvent.getLastResult() != null) {
HttpResponse response = executionAttemptedEvent.getLastResult();
Expand Down
87 changes: 11 additions & 76 deletions java/test/org/openqa/selenium/remote/http/RetryRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package org.openqa.selenium.remote.http;

import static java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT;
import static java.net.HttpURLConnection.HTTP_GATEWAY_TIMEOUT;
import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
import static java.net.HttpURLConnection.HTTP_OK;
import static java.net.HttpURLConnection.HTTP_UNAVAILABLE;
Expand Down Expand Up @@ -56,7 +55,8 @@ public void setUp() throws MalformedURLException {
ClientConfig.defaultConfig()
.baseUrl(URI.create("http://localhost:2345").toURL())
.withRetries()
.readTimeout(Duration.ofSeconds(1));
.readTimeout(Duration.ofSeconds(1))
.connectionTimeout(Duration.ofSeconds(1));
client = HttpClient.Factory.createDefault().createClient(config);
}

Expand All @@ -82,8 +82,8 @@ void canReturnAppropriateFallbackResponse() {
throw new TimeoutException();
});

Assertions.assertEquals(
HTTP_GATEWAY_TIMEOUT, handler1.execute(new HttpRequest(GET, "/")).getStatus());
Assertions.assertThrows(
TimeoutException.class, () -> handler1.execute(new HttpRequest(GET, "/")));

HttpHandler handler2 =
new RetryRequest()
Expand All @@ -96,12 +96,11 @@ void canReturnAppropriateFallbackResponse() {
@Test
void canReturnAppropriateFallbackResponseWithMultipleThreads()
throws InterruptedException, ExecutionException {
HttpHandler handler1 =
new RetryRequest()
.andFinally(
(HttpRequest request) -> {
throw new TimeoutException();
});
AppServer server = new NettyAppServer(req -> new HttpResponse());

URI uri = URI.create(server.whereIs("/"));
HttpRequest connectionTimeoutRequest =
new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort()));
titusfortner marked this conversation as resolved.
Show resolved Hide resolved

HttpHandler handler2 =
new RetryRequest()
Expand All @@ -110,12 +109,12 @@ void canReturnAppropriateFallbackResponseWithMultipleThreads()
ExecutorService executorService = Executors.newFixedThreadPool(2);
List<Callable<HttpResponse>> tasks = new ArrayList<>();

tasks.add(() -> handler1.execute(new HttpRequest(GET, "/")));
tasks.add(() -> client.execute(connectionTimeoutRequest));
tasks.add(() -> handler2.execute(new HttpRequest(GET, "/")));

List<Future<HttpResponse>> results = executorService.invokeAll(tasks);

Assertions.assertEquals(HTTP_GATEWAY_TIMEOUT, results.get(0).get().getStatus());
Assertions.assertEquals(HTTP_CLIENT_TIMEOUT, results.get(0).get().getStatus());

Assertions.assertEquals(HTTP_UNAVAILABLE, results.get(1).get().getStatus());
}
Expand Down Expand Up @@ -265,70 +264,6 @@ void shouldGetTheErrorResponseOnServerUnavailableError() {
server.stop();
}

@Test
void shouldBeAbleToRetryARequestOnTimeout() {
AtomicInteger count = new AtomicInteger(0);
AppServer server =
new NettyAppServer(
req -> {
count.incrementAndGet();
if (count.get() <= 3) {
try {
Thread.sleep(2000);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
return new HttpResponse();
});
server.start();

URI uri = URI.create(server.whereIs("/"));
HttpRequest request =
new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort()));

HttpResponse response = client.execute(request);
assertThat(response).extracting(HttpResponse::getStatus).isEqualTo(HTTP_OK);
assertThat(count.get()).isEqualTo(4);

server.stop();
}

@Test
void shouldBeAbleToGetErrorResponseOnRequestTimeout() {
AtomicInteger count = new AtomicInteger(0);
AppServer server =
new NettyAppServer(
req -> {
count.incrementAndGet();
throw new TimeoutException();
});
server.start();

URI uri = URI.create(server.whereIs("/"));
HttpRequest request =
new HttpRequest(GET, String.format(REQUEST_PATH, uri.getHost(), uri.getPort()));

HttpResponse response = client.execute(request);

// The NettyAppServer passes the request through ErrorFilter.
// This maps the timeout exception to HTTP response code 500 and HTTP response body containing
// "timeout".
// RetryRequest retries if it gets a TimeoutException only.
// Parsing and inspecting the response body each time if HTTP response code 500 is not
// efficient.
// A potential solution can be updating the ErrorCodec to reflect the appropriate HTTP code
// (this is a breaking change).
// RetryRequest can then inspect just the HTTP response status code and retry.

assertThat(response).extracting(HttpResponse::getStatus).isEqualTo(HTTP_INTERNAL_ERROR);

// This should ideally be more than the number of retries configured i.e. greater than 3
assertThat(count.get()).isEqualTo(1);

server.stop();
}

@Test
void shouldBeAbleToRetryARequestOnConnectionFailure() {
AppServer server = new NettyAppServer(req -> new HttpResponse());
Expand Down
Loading