Skip to content

Commit

Permalink
StashApiClient: Run HTTP requests in the main thread (#137)
Browse files Browse the repository at this point in the history 
* StashApiClient: Run HTTP requests in the main thread

Using separate threads adds complexity for little benefit. Socket timeout
should take care of slow HTTP connections.

JENKINS-30558 doesn't provide any evidence that a separate timeout for
requests was needed. The request timeout and the socket timeout were added
in the same commit 80431f4.

This change eliminates ExecutionException as an exception thrown by
StashApiClient, replacing it with more specific exceptions. Update unit
tests for the new behavior.

* StashApiClient: Declare httpTask and thread where they are initialized
  • Loading branch information
@proski @jakub-bochenski
proski authored and jakub-bochenski committed Nov 29, 2019
1 parent dc86da7 commit bf508bf
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 160 deletions.
159 changes: 26 additions & 133 deletions src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/stash/StashApiClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;
Expand Down Expand Up @@ -47,10 +42,6 @@
/** Created by Nathan McCarthy */
public class StashApiClient {

// Request timeout: maximum time between sending an HTTP request and receiving
// a response to it from the server.
private static final int HTTP_REQUEST_TIMEOUT_SECONDS = 60;

// Connection timeout: maximum time for connecting to the HTTP server.
private static final int HTTP_CONNECTION_TIMEOUT_SECONDS = 15;

Expand Down Expand Up @@ -247,53 +238,21 @@ private String getRequest(String path) throws StashApiException {
request.addHeader("Connection", "close");

String response = null;
FutureTask<String> httpTask = null;
Thread thread;
try {
request.addHeader(new BasicScheme().authenticate(credentials, request, null));
HttpResponse httpResponse = client.execute(request);
int responseCode = httpResponse.getStatusLine().getStatusCode();
if (!validResponseCode(responseCode)) {
logger.log(Level.SEVERE, "Failing to get response from Stash PR GET" + request.getURI());
throw new StashApiException(
"Didn't get a 200 response from Stash PR GET! Response; '"
+ responseCode
+ "' with message; "
+ httpResponse.getStatusLine().getReasonPhrase());
}

// Run the HTTP request in a future task so we have the opportunity
// to cancel it if it gets hung up; which is possible if stuck at
// socket native layer. see issue JENKINS-30558
httpTask =
new FutureTask<String>(
new Callable<String>() {

private CloseableHttpClient client;
private HttpGet request;

@Override
public String call() throws StashApiException, IOException {
HttpResponse httpResponse = client.execute(request);
int responseCode = httpResponse.getStatusLine().getStatusCode();
if (!validResponseCode(responseCode)) {
logger.log(
Level.SEVERE,
"Failing to get response from Stash PR GET" + request.getURI());
throw new StashApiException(
"Didn't get a 200 response from Stash PR GET! Response; '"
+ responseCode
+ "' with message; "
+ httpResponse.getStatusLine().getReasonPhrase());
}

return entityAsString(httpResponse);
}

public Callable<String> init(CloseableHttpClient client, HttpGet request) {
this.client = client;
this.request = request;
return this;
}
}.init(client, request));
thread = new Thread(httpTask);
thread.start();
response = httpTask.get(HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);

} catch (TimeoutException e) {
request.abort();
throw new StashApiException("Timeout in GET request", e);
} catch (AuthenticationException | ExecutionException | InterruptedException e) {
response = entityAsString(httpResponse);
} catch (AuthenticationException | IOException e) {
throw new StashApiException("Exception in GET request", e);
} finally {
request.releaseConnection();
Expand All @@ -312,43 +271,10 @@ public void deleteRequest(String path) throws StashApiException {
request.setHeader("Connection", "close");

int response = -1;
FutureTask<Integer> httpTask = null;
Thread thread;

try {
request.addHeader(new BasicScheme().authenticate(credentials, request, null));

// Run the HTTP request in a future task so we have the opportunity
// to cancel it if it gets hung up; which is possible if stuck at
// socket native layer. see issue JENKINS-30558
httpTask =
new FutureTask<Integer>(
new Callable<Integer>() {

private CloseableHttpClient client;
private HttpDelete request;

@Override
public Integer call() throws StashApiException, IOException {
int response = -1;
response = client.execute(request).getStatusLine().getStatusCode();
return response;
}

public Callable<Integer> init(CloseableHttpClient client, HttpDelete request) {
this.client = client;
this.request = request;
return this;
}
}.init(client, request));
thread = new Thread(httpTask);
thread.start();
response = httpTask.get(HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);

} catch (TimeoutException e) {
request.abort();
throw new StashApiException("Timeout in DELETE request", e);
} catch (AuthenticationException | ExecutionException | InterruptedException e) {
response = client.execute(request).getStatusLine().getStatusCode();
} catch (AuthenticationException | IOException e) {
throw new StashApiException("Exception in DELETE request", e);
} finally {
request.releaseConnection();
Expand Down Expand Up @@ -383,54 +309,21 @@ private String postRequest(String path, String comment) throws StashApiException
}

String response = "";
FutureTask<String> httpTask = null;
Thread thread;

try {
request.addHeader(new BasicScheme().authenticate(credentials, request, null));
HttpResponse httpResponse = client.execute(request);
int responseCode = httpResponse.getStatusLine().getStatusCode();
if (!validResponseCode(responseCode)) {
logger.log(Level.SEVERE, "Failing to get response from Stash PR POST" + request.getURI());
throw new StashApiException(
"Didn't get a 200 response from Stash PR POST! Response; '"
+ responseCode
+ "' with message; "
+ httpResponse.getStatusLine().getReasonPhrase());
}

// Run the HTTP request in a future task so we have the opportunity
// to cancel it if it gets hung up; which is possible if stuck at
// socket native layer. see issue JENKINS-30558
httpTask =
new FutureTask<String>(
new Callable<String>() {

private CloseableHttpClient client;
private HttpPost request;

@Override
public String call() throws StashApiException, IOException {
HttpResponse httpResponse = client.execute(request);
int responseCode = httpResponse.getStatusLine().getStatusCode();
if (!validResponseCode(responseCode)) {
logger.log(
Level.SEVERE,
"Failing to get response from Stash PR POST" + request.getURI());
throw new StashApiException(
"Didn't get a 200 response from Stash PR POST! Response; '"
+ responseCode
+ "' with message; "
+ httpResponse.getStatusLine().getReasonPhrase());
}

return entityAsString(httpResponse);
}

public Callable<String> init(CloseableHttpClient client, HttpPost request) {
this.client = client;
this.request = request;
return this;
}
}.init(client, request));
thread = new Thread(httpTask);
thread.start();
response = httpTask.get(HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);

} catch (TimeoutException e) {
request.abort();
throw new StashApiException("Timeout in POST request", e);
} catch (AuthenticationException | ExecutionException | InterruptedException e) {
response = entityAsString(httpResponse);
} catch (AuthenticationException | IOException e) {
throw new StashApiException("Exception in POST request", e);
} finally {
request.releaseConnection();
Expand Down

0 comments on commit bf508bf

Please sign in to comment.