honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Tanguy Leroux 2017-02-28 09:33:51 +01:00 committed by GitHub
parent e866da580c
commit 9da8531f60
2 changed files with 118 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

64
client/rest/src/main/java/org/elasticsearch/client/RestClient.java
View File

@ -289,40 +289,44 @@ public class RestClient implements Closeable {
public void performRequestAsync(String method, String endpoint, Map<String, String> params,
HttpEntity entity, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
ResponseListener responseListener, Header... headers) {
Objects.requireNonNull(params, "params must not be null");
Map<String, String> requestParams = new HashMap<>(params);
//ignore is a special parameter supported by the clients, shouldn't be sent to es
String ignoreString = requestParams.remove("ignore");
Set<Integer> ignoreErrorCodes;
if (ignoreString == null) {
if (HttpHead.METHOD_NAME.equals(method)) {
//404 never causes error if returned for a HEAD request
ignoreErrorCodes = Collections.singleton(404);
try {
Objects.requireNonNull(params, "params must not be null");
Map<String, String> requestParams = new HashMap<>(params);
//ignore is a special parameter supported by the clients, shouldn't be sent to es
String ignoreString = requestParams.remove("ignore");
Set<Integer> ignoreErrorCodes;
if (ignoreString == null) {
if (HttpHead.METHOD_NAME.equals(method)) {
//404 never causes error if returned for a HEAD request
ignoreErrorCodes = Collections.singleton(404);
} else {
ignoreErrorCodes = Collections.emptySet();
}
} else {
ignoreErrorCodes = Collections.emptySet();
}
} else {
String[] ignoresArray = ignoreString.split(",");
ignoreErrorCodes = new HashSet<>();
if (HttpHead.METHOD_NAME.equals(method)) {
//404 never causes error if returned for a HEAD request
ignoreErrorCodes.add(404);
}
for (String ignoreCode : ignoresArray) {
try {
ignoreErrorCodes.add(Integer.valueOf(ignoreCode));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e);
String[] ignoresArray = ignoreString.split(",");
ignoreErrorCodes = new HashSet<>();
if (HttpHead.METHOD_NAME.equals(method)) {
//404 never causes error if returned for a HEAD request
ignoreErrorCodes.add(404);
}
for (String ignoreCode : ignoresArray) {
try {
ignoreErrorCodes.add(Integer.valueOf(ignoreCode));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("ignore value should be a number, found [" + ignoreString + "] instead", e);
}
}
}
URI uri = buildUri(pathPrefix, endpoint, requestParams);
HttpRequestBase request = createHttpRequest(method, uri, entity);
setHeaders(request, headers);
FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener);
long startTime = System.nanoTime();
performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory,
failureTrackingResponseListener);
} catch (Exception e) {
responseListener.onFailure(e);
}
URI uri = buildUri(pathPrefix, endpoint, requestParams);
HttpRequestBase request = createHttpRequest(method, uri, entity);
setHeaders(request, headers);
FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(responseListener);
long startTime = System.nanoTime();
performRequestAsync(startTime, nextHost(), request, ignoreErrorCodes, httpAsyncResponseConsumerFactory,
failureTrackingResponseListener);
}
private void performRequestAsync(final long startTime, final HostTuple<Iterator<HttpHost>> hostTuple, final HttpRequestBase request,

84
client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java Normal file
View File

@ -0,0 +1,84 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you 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.elasticsearch.client;
import org.apache.http.Header;
import org.apache.http.HttpHost;
import org.apache.http.impl.nio.client.CloseableHttpAsyncClient;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
public class RestClientTests extends RestClientTestCase {
public void testPerformAsyncWithUnsupportedMethod() throws Exception {
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
try (RestClient restClient = createRestClient()) {
restClient.performRequestAsync("unsupported", randomAsciiOfLength(5), listener);
listener.get();
fail("should have failed because of unsupported method");
} catch (UnsupportedOperationException exception) {
assertEquals("http method not supported: unsupported", exception.getMessage());
}
}
public void testPerformAsyncWithNullParams() throws Exception {
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
try (RestClient restClient = createRestClient()) {
restClient.performRequestAsync(randomAsciiOfLength(5), randomAsciiOfLength(5), null, listener);
listener.get();
fail("should have failed because of null parameters");
} catch (NullPointerException exception) {
assertEquals("params must not be null", exception.getMessage());
}
}
public void testPerformAsyncWithNullHeaders() throws Exception {
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
try (RestClient restClient = createRestClient()) {
restClient.performRequestAsync("GET", randomAsciiOfLength(5), listener, null);
listener.get();
fail("should have failed because of null headers");
} catch (NullPointerException exception) {
assertEquals("request headers must not be null", exception.getMessage());
}
}
public void testPerformAsyncWithWrongEndpoint() throws Exception {
RestClient.SyncResponseListener listener = new RestClient.SyncResponseListener(10000);
try (RestClient restClient = createRestClient()) {
restClient.performRequestAsync("GET", "::http:///", listener);
listener.get();
fail("should have failed because of wrong endpoint");
} catch (IllegalArgumentException exception) {
assertEquals("Expected scheme name at index 0: ::http:///", exception.getMessage());
}
}
private static RestClient createRestClient() {
HttpHost[] hosts = new HttpHost[]{new HttpHost("localhost", 9200)};
return new RestClient(mock(CloseableHttpAsyncClient.class), randomLongBetween(1_000, 30_000), new Header[]{}, hosts, null, null);
}
}