Skip to content

Refactor attempt #3 #339

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

Merged
merged 11 commits into from
Oct 19, 2013
Merged

Refactor attempt #3 #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cc38f5d
Major refactor on current code base including:
sweetlilmre Oct 17, 2013
cda3f4a
Fix minor formatting issue
sweetlilmre Oct 17, 2013
6e6569f
Added another cancellation check
sweetlilmre Oct 17, 2013
b20f6f5
Fix override issue in sample app
sweetlilmre Oct 17, 2013
3dfc05e
removed redundant methods
sweetlilmre Oct 17, 2013
f279cb5
Fixed examples
sweetlilmre Oct 17, 2013
a65c7d6
Clean rebase for proposed changes
sweetlilmre Oct 18, 2013
2988325
Examples and other files rebase
sweetlilmre Oct 18, 2013
6fe5ce6
Refactor preserving call chain for backward compatibility
sweetlilmre Oct 18, 2013
dfd8661
Rebasing to latest commit from upstream
sweetlilmre Oct 18, 2013
4b90839
Merge remote-tracking branch 'upstream/master'
sweetlilmre Oct 18, 2013
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
2 changes: 1 addition & 1 deletion library/src/com/loopj/android/http/AsyncHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,7 @@ private HttpEntity paramsToEntity(RequestParams params, AsyncHttpResponseHandler
}
} catch (Throwable t) {
if (responseHandler != null)
responseHandler.sendFailureMessage(0, null, t, (String) null);
responseHandler.sendFailureMessage(0, null, (byte[]) null, t);
else
t.printStackTrace();
}
Expand Down
132 changes: 51 additions & 81 deletions library/src/com/loopj/android/http/AsyncHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,118 +38,88 @@ class AsyncHttpRequest implements Runnable {
private final HttpContext context;
private final HttpUriRequest request;
private final AsyncHttpResponseHandler responseHandler;
private boolean isBinaryRequest;
private int executionCount;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great refactoring, it has been driving me nuts too


public AsyncHttpRequest(AbstractHttpClient client, HttpContext context, HttpUriRequest request, AsyncHttpResponseHandler responseHandler) {
this.client = client;
this.context = context;
this.request = request;
this.responseHandler = responseHandler;
if (responseHandler instanceof BinaryHttpResponseHandler) {
this.isBinaryRequest = true;
}
}

@Override
public void run() {
try {
if (responseHandler != null) {
responseHandler.sendStartMessage();
}
if (responseHandler != null) {
responseHandler.sendStartMessage();
}

try {
makeRequestWithRetries();

if (responseHandler != null) {
responseHandler.sendFinishMessage();
}
} catch (IOException e) {
if (responseHandler != null) {
responseHandler.sendFinishMessage();
if (this.isBinaryRequest) {
responseHandler.sendFailureMessage(e, (byte[]) null);
} else {
responseHandler.sendFailureMessage(e, (String) null);
}
responseHandler.sendFailureMessage(0, null, (byte[]) null, e);
}
}

if (responseHandler != null) {
responseHandler.sendFinishMessage();
}
}

private void makeRequest() throws IOException, InterruptedException {
private void makeRequest() throws IOException {
if (!Thread.currentThread().isInterrupted()) {
try {
// Fixes #115
if (request.getURI().getScheme() == null)
throw new MalformedURLException("No valid URI scheme was provided");
HttpResponse response = client.execute(request, context);
if (!Thread.currentThread().isInterrupted()) {
if (responseHandler != null) {
responseHandler.sendResponseMessage(response);
}
} else {
throw new InterruptedException("makeRequest was interrupted");
}
} catch (IOException e) {
if (!Thread.currentThread().isInterrupted()) {
throw e;
// Fixes #115
if (request.getURI().getScheme() == null) {
// subclass of IOException so processed in the caller
throw new MalformedURLException("No valid URI scheme was provided");
}

HttpResponse response = client.execute(request, context);

if (!Thread.currentThread().isInterrupted()) {
if (responseHandler != null) {
responseHandler.sendResponseMessage(response);
}
}
}
}

private void makeRequestWithRetries() throws ConnectException {
// This is an additional layer of retry logic lifted from droid-fu
// See: https://github.com/kaeppler/droid-fu/blob/master/src/main/java/com/github/droidfu/http/BetterHttpRequestBase.java
private void makeRequestWithRetries() throws IOException {
boolean retry = true;
IOException cause = null;
HttpRequestRetryHandler retryHandler = client.getHttpRequestRetryHandler();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you removed throwing request interruption and so calling onFailed in handler subclass, how will this be handled now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interruption check will cause a silent exit (as intended from cancelRequests with the mayInterruptIfRunning flag set)

For any other exceptions: they are caught in the base handler and wrapped as IOExceptions.

while (retry) {
try {
makeRequest();
return;
} catch (ClientProtocolException e) {
if (responseHandler != null) {
responseHandler.sendFailureMessage(e, "cannot repeat the request");
}
return;
} catch (UnknownHostException e) {
if (responseHandler != null) {
responseHandler.sendFailureMessage(e, "can't resolve host");
try
{
while (retry) {
try {
makeRequest();
return;
} catch (UnknownHostException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sincerely not only case of connectivity switching, first it happens if you don't enlist INTERNET permission, second, the DNS or data-flow on the way can be corrupted somehow, and this shouldn't drive you into more than 2-3 tries to resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bugged me for ages (our client needed to be resilient to network switch) and this seemed the best of a bad set of ways to handle it. Hopefully this will catch most base issues. It would be interesting to try and develop some definitive test cases for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I like it that way, we'll see how it behaves in the wild.

// switching between WI-FI and mobile data networks can cause a retry which then results in an UnknownHostException
// while the WI-FI is initialising. The retry logic will be invoked here, if this is NOT the first retry
// (to assist in genuine cases of unknown host) which seems better than outright failure
cause = new IOException("UnknownHostException exception: " + e.getMessage());
retry = (executionCount > 0) && retryHandler.retryRequest(cause, ++executionCount, context);
} catch (NullPointerException e) {
// there's a bug in HttpClient 4.0.x that on some occasions causes
// DefaultRequestExecutor to throw an NPE, see
// http://code.google.com/p/android/issues/detail?id=5255
cause = new IOException("NPE in HttpClient: " + e.getMessage());
retry = retryHandler.retryRequest(cause, ++executionCount, context);
} catch (IOException e) {
cause = e;
retry = retryHandler.retryRequest(cause, ++executionCount, context);
}
return;
} catch (ConnectTimeoutException e) {
if (responseHandler != null) {
responseHandler.sendFailureMessage(e, "connection timed out");
}
} catch (SocketException e) {
// Added to detect host unreachable
if (responseHandler != null) {
responseHandler.sendFailureMessage(e, "can't resolve host");
if(retry && (responseHandler != null)) {
responseHandler.sendRetryMessage();
}
return;
} catch (SocketTimeoutException e) {
if (responseHandler != null) {
responseHandler.sendFailureMessage(e, "socket time out");
}
return;
} catch (IOException e) {
cause = e;
retry = retryHandler.retryRequest(cause, ++executionCount, context);
} catch (NullPointerException e) {
// there's a bug in HttpClient 4.0.x that on some occasions causes
// DefaultRequestExecutor to throw an NPE, see
// http://code.google.com/p/android/issues/detail?id=5255
cause = new IOException("NPE in HttpClient" + e.getMessage());
retry = retryHandler.retryRequest(cause, ++executionCount, context);
} catch (InterruptedException e) {
cause = new IOException("Request was interrupted while executing");
retry = retryHandler.retryRequest(cause, ++executionCount, context);
}
} catch (Exception e) {
// catch anything else to ensure failure message is propagated
cause = new IOException("Unhandled exception: " + e.getMessage());
}

// no retries left, crap out with exception
ConnectException ex = new ConnectException();
ex.initCause(cause);
throw ex;

// cleaned up to throw IOException
throw(cause);
}
}
Loading