Merge pull request #127 from cloudsufi/bugfix/PLUGIN-1928

PLUGIN-1928: Set connect & read timeout to http client and retry ConnectTimeoutException & SocketException

Author: sgarg-CS

src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowAPIException.java

@@ -4,8 +4,10 @@
 
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.oltu.oauth2.common.exception.OAuthSystemException;
 
+import java.net.SocketException;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
@@ -65,6 +67,8 @@ public boolean isErrorRetryable() {
     }
     Throwable t = this.getCause();
     return t instanceof OAuthSystemException
+        || t instanceof ConnectTimeoutException
+        || t instanceof SocketException
         || (this.getMessage() != null
         && this.getMessage().contains(ServiceNowConstants.MAXIMUM_EXECUTION_TIME_EXCEEDED))
         || RETRYABLE_CODES.contains(getStatusCode());

src/main/java/io/cdap/plugin/servicenow/restapi/RestAPIClient.java

@@ -24,9 +24,11 @@
 import com.github.rholder.retry.WaitStrategies;
 import io.cdap.plugin.servicenow.apiclient.ServiceNowAPIException;
 import io.cdap.plugin.servicenow.util.ServiceNowConstants;
+import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.oltu.oauth2.client.OAuthClient;
@@ -41,6 +43,8 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.net.SocketException;
+import java.util.Collections;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -50,6 +54,17 @@
  */
 public abstract class RestAPIClient {
   private static final Logger LOG = LoggerFactory.getLogger(RestAPIClient.class);
+  
+  /* Connect Timout in ms for establishing the conenction with the server */
+  private static final int DEFAULT_CONNECT_TIMEOUT_MS = 120000;
+
+  /* Read Timeout in ms for waiting for data after the connection is established */
+  private static final int DEFAULT_READ_TIMEOUT_MS = 300000;
+
+  private static final RequestConfig requestConfig = RequestConfig.custom()
+    .setConnectTimeout(DEFAULT_CONNECT_TIMEOUT_MS)
+    .setSocketTimeout(DEFAULT_READ_TIMEOUT_MS)
+    .build();
 
   /**
    * Executes the Rest API request and returns the response.
@@ -61,10 +76,13 @@ public RestAPIResponse executeGet(RestAPIRequest request) throws IOException {
     HttpGet httpGet = new HttpGet(request.getUrl());
     request.getHeaders().entrySet().forEach(e -> httpGet.addHeader(e.getKey(), e.getValue()));
 
-    try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) {
+    try (CloseableHttpClient httpClient = HttpClientBuilder.create().setDefaultRequestConfig(requestConfig).build()) {
       try (CloseableHttpResponse httpResponse = httpClient.execute(httpGet)) {
         return RestAPIResponse.parse(httpResponse, request.getResponseHeaders());
       }
+    } catch (ConnectTimeoutException | SocketException e) {
+      ServiceNowAPIException exception = new ServiceNowAPIException(e, null);
+      return new RestAPIResponse(Collections.emptyMap(), null, exception);
     }
   }
 
@@ -136,7 +154,7 @@ public RestAPIResponse executePost(RestAPIRequest request) throws IOException {
     // We're retrying all transport exceptions while executing the HTTP POST method and the generic transport
     // exceptions in HttpClient are represented by the standard java.io.IOException class
     // https://hc.apache.org/httpclient-legacy/exception-handling.html
-    try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) {
+    try (CloseableHttpClient httpClient = HttpClientBuilder.create().setDefaultRequestConfig(requestConfig).build()) {
       try (CloseableHttpResponse httpResponse = httpClient.execute(httpPost)) {
         return RestAPIResponse.parse(httpResponse, request.getResponseHeaders());
       }

src/test/java/io/cdap/plugin/servicenow/restapi/RestAPIClientTest.java

@@ -6,6 +6,7 @@
 import org.apache.http.HttpStatus;
 import org.apache.http.StatusLine;
 import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.message.BasicStatusLine;
@@ -19,6 +20,7 @@
 import org.powermock.modules.junit4.PowerMockRunner;
 
 import java.io.IOException;
+import java.net.SocketException;
 
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({
@@ -102,4 +104,49 @@ public void testExecuteGet_StatusOk() throws IOException {
     ServiceNowTableAPIClientImpl client = new ServiceNowTableAPIClientImpl(config, true);
     client.executeGet(request);
   }
+
+  @Test
+  public void testExecuteGet_throwConnectTimeoutException_markAsRetryable() throws IOException {
+    CloseableHttpClient httpClient = Mockito.mock(CloseableHttpClient.class);
+    HttpClientBuilder httpClientBuilder = Mockito.mock(HttpClientBuilder.class);
+    PowerMockito.mockStatic(HttpClientBuilder.class);
+    PowerMockito.when(HttpClientBuilder.create()).thenReturn(httpClientBuilder);
+    Mockito.when(httpClientBuilder.build()).thenReturn(httpClient);
+    Mockito.when(httpClient.execute(Mockito.any()))
+      .thenThrow(new ConnectTimeoutException("Connection timed out"));
+
+    ServiceNowTableAPIRequestBuilder builder = new ServiceNowTableAPIRequestBuilder("url");
+    RestAPIRequest request = builder.build();
+
+    ServiceNowConnectorConfig config = Mockito.mock(ServiceNowConnectorConfig.class);
+    ServiceNowTableAPIClientImpl client = new ServiceNowTableAPIClientImpl(config, true);
+    RestAPIResponse actualResponse = client.executeGet(request);
+    Assert.assertNotNull(actualResponse.getException());
+    Assert.assertTrue(actualResponse.getException().isErrorRetryable());
+    Throwable ex = actualResponse.getException().getCause();
+    Assert.assertTrue("Expected ConnectTimeoutException or similar, got: " + ex,
+      ex instanceof ConnectTimeoutException || ex.getMessage().toLowerCase().contains("Connection timed out"));
+  }
+
+  @Test
+  public void testExecuteGet_throwSocketException_markAsRetryable() throws IOException {
+    CloseableHttpClient httpClient = Mockito.mock(CloseableHttpClient.class);
+    HttpClientBuilder httpClientBuilder = Mockito.mock(HttpClientBuilder.class);
+    PowerMockito.mockStatic(HttpClientBuilder.class);
+    PowerMockito.when(HttpClientBuilder.create()).thenReturn(httpClientBuilder);
+    Mockito.when(httpClientBuilder.build()).thenReturn(httpClient);
+    Mockito.when(httpClient.execute(Mockito.any()))
+      .thenThrow(new SocketException());
+
+    ServiceNowTableAPIRequestBuilder builder = new ServiceNowTableAPIRequestBuilder("url");
+    RestAPIRequest request = builder.build();
+
+    ServiceNowConnectorConfig config = Mockito.mock(ServiceNowConnectorConfig.class);
+    ServiceNowTableAPIClientImpl client = new ServiceNowTableAPIClientImpl(config, true);
+    RestAPIResponse actualResponse = client.executeGet(request);
+    Assert.assertNotNull(actualResponse.getException());
+    Assert.assertTrue(actualResponse.getException().isErrorRetryable());
+    Throwable ex = actualResponse.getException().getCause();
+    Assert.assertTrue("Expected SocketException or similar, got: " + ex, ex instanceof SocketException);
+  }
 }