Skip to content

Commit 9da8531

Browse files
tlrxtlrx
authored
RestClient asynchronous execution should not throw exceptions (#23307)
The current implementation of RestClient.performAsync() methods can throw exceptions before the request is asynchronously executed. Since it only throws unchecked exceptions, it's easy for the user/dev to forget to catch them. Instead I think async methods should never throw exceptions and should always call the listener onFailure() method.
1 parent e866da5 commit 9da8531

File tree

2 files changed

+118
-30
lines changed

2 files changed

+118
-30
lines changed

‎client/rest/src/main/java/org/elasticsearch/client/RestClient.java‎

Lines changed: 34 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -289,40 +289,44 @@ public void performRequestAsync(String method, String endpoint, Map<String, Stri
289289
public void performRequestAsync(String method, String endpoint, Map<String, String> params,
290290
HttpEntity entity, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
291291
ResponseListener responseListener, Header... headers) {
292-
Objects.requireNonNull(params, "params must not be null");
293-
Map<String, String> requestParams = new HashMap<>(params);
294-
//ignore is a special parameter supported by the clients, shouldn't be sent to es
295-
String ignoreString = requestParams.remove("ignore");
296-
Set<Integer> ignoreErrorCodes;
297-
if (ignoreString == null) {
298-
if (HttpHead.METHOD_NAME.equals(method)) {
299-
//404 never causes error if returned for a HEAD request
300-
ignoreErrorCodes = Collections.singleton(404);
292+
try {
293+
Objects.requireNonNull(params, "params must not be null");
294+
Map<String, String> requestParams = new HashMap<>(params);
295+
//ignore is a special parameter supported by the clients, shouldn't be sent to es
296+
String ignoreString = requestParams.remove("ignore");
297+
Set<Integer> ignoreErrorCodes;
298+
if (ignoreString == null) {
299+
if (HttpHead.METHOD_NAME.equals(method)) {
300+
//404 never causes error if returned for a HEAD request
301+
ignoreErrorCodes = Collections.singleton(404);
302+
} else {
303+
ignoreErrorCodes = Collections.emptySet();
304+
}
301305
} else {
302-
ignoreErrorCodes = Collections.emptySet();
303-
}
304-
} else {
305-
String[] ignoresArray = ignoreString.split(",");
306-
ignoreErrorCodes = new HashSet<>();
307-
if (HttpHead.METHOD_NAME.equals(method)) {
308-
//404 never causes error if returned for a HEAD request
309-
ignoreErrorCodes.add(404);
310-
}
311-
for (String ignoreCode : ignoresArray) {
312-
try {
313-
ignoreErrorCodes.add(Integer.valueOf(ignoreCode));
314-
} catch (NumberFormatException e) {
315-
throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e);
306+
String[] ignoresArray = ignoreString.split(",");
307+
ignoreErrorCodes = new HashSet<>();
308+
if (HttpHead.METHOD_NAME.equals(method)) {
309+
//404 never causes error if returned for a HEAD request
310+
ignoreErrorCodes.add(404);
311+
}
312+
for (String ignoreCode : ignoresArray) {
313+
try {
314+
ignoreErrorCodes.add(Integer.valueOf(ignoreCode));
315+
} catch (NumberFormatException e) {
316+
throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e);
317+
}
316318
}
317319
}
320+
URI uri = buildUri(pathPrefix, endpoint, requestParams);
321+
HttpRequestBase request = createHttpRequest(method, uri, entity);
322+
setHeaders(request, headers);
323+
FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener);
324+
long startTime = System.nanoTime();
325+
performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory,
326+
failureTrackingResponseListener);
327+
} catch (Exception e) {
328+
responseListener.onFailure(e);
318329
}
319-
URI uri = buildUri(pathPrefix, endpoint, requestParams);
320-
HttpRequestBase request = createHttpRequest(method, uri, entity);
321-
setHeaders(request, headers);
322-
FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener);
323-
long startTime = System.nanoTime();
324-
performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory,
325-
failureTrackingResponseListener);
326330
}
327331

328332
private void performRequestAsync(final long startTime, final HostTuple<Iterator<HttpHost>> hostTuple, final HttpRequestBase request,

‎client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.client;
21+
22+
import org.apache.http.Header;
23+
import org.apache.http.HttpHost;
24+
import org.apache.http.impl.nio.client.CloseableHttpAsyncClient;
25+
26+
import static org.junit.Assert.assertEquals;
27+
import static org.junit.Assert.fail;
28+
import static org.mockito.Mockito.mock;
29+
30+
public class RestClientTests extends RestClientTestCase {
31+
32+
public void testPerformAsyncWithUnsupportedMethod() throws Exception {
33+
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
34+
try (RestClient restClient = createRestClient()) {
35+
restClient.performRequestAsync("unsupported", randomAsciiOfLength(5), listener);
36+
listener.get();
37+
38+
fail("should have failed because of unsupported method");
39+
} catch (UnsupportedOperationException exception) {
40+
assertEquals("http method not supported: unsupported", exception.getMessage());
41+
}
42+
}
43+
44+
public void testPerformAsyncWithNullParams() throws Exception {
45+
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
46+
try (RestClient restClient = createRestClient()) {
47+
restClient.performRequestAsync(randomAsciiOfLength(5), randomAsciiOfLength(5), null, listener);
48+
listener.get();
49+
50+
fail("should have failed because of null parameters");
51+
} catch (NullPointerException exception) {
52+
assertEquals("params must not be null", exception.getMessage());
53+
}
54+
}
55+
56+
public void testPerformAsyncWithNullHeaders() throws Exception {
57+
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
58+
try (RestClient restClient = createRestClient()) {
59+
restClient.performRequestAsync("GET", randomAsciiOfLength(5), listener, null);
60+
listener.get();
61+
62+
fail("should have failed because of null headers");
63+
} catch (NullPointerException exception) {
64+
assertEquals("request headers must not be null", exception.getMessage());
65+
}
66+
}
67+
68+
public void testPerformAsyncWithWrongEndpoint() throws Exception {
69+
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
70+
try (RestClient restClient = createRestClient()) {
71+
restClient.performRequestAsync("GET", "::http:///", listener);
72+
listener.get();
73+
74+
fail("should have failed because of wrong endpoint");
75+
} catch (IllegalArgumentException exception) {
76+
assertEquals("Expected scheme name at index 0: ::http:///", exception.getMessage());
77+
}
78+
}
79+
80+
private static RestClient createRestClient() {
81+
HttpHost[] hosts = new HttpHost[]{new HttpHost("localhost", 9200)};
82+
return new RestClient(mock(CloseableHttpAsyncClient.class), randomLongBetween(1_000, 30_000), new Header[]{}, hosts, null, null);
83+
}
84+
}

0 commit comments

Comments
 (0)