Skip to content

Commit

Permalink
Android: Several fixes and improvements to UploadTask.
Browse files Browse the repository at this point in the history 
This is a large commit, necessitated by the wrapping of most of `doWork()` by a large `try`/`catch` block. Which aims to fix several long-standing issues and improve our ability to understand the code and make improvements in hte long term.

First, comments were added. Members of my team (myself included) aren't used to looking at this, so we took notes about what goes on in the class to help us better understand what is going on.

Second, an outer `try` was added so that we could figure out what exceptions were causing our use cases to fail. It turns out that sometimes the `response` can be null—without throwing an exception!

Third, some of the logic of how workers pick up uploads and processed them was updated (and hopefully improved).

- This includes a change which marks upload as pending. In our experience, an upload which failed could continue to be loaded as the "next" upload to process, thereby making no future uploads work until the applications cache was removed. We also ran into an issue where sometimes uploads would execute 2 or 3 times, this change ensures that an upload only gets processed one time. In our application, we handle retries ourselves, so this works with our use-case.
- Avoid some cases where we would "return" rather than continue. I think a problem we found was that due to errors in how the uploads worked, we would often get a "backlog" of uploads that needed to be sent. These changes may need to be re-thought, but essentially, we allow each worker to continue until there are no more uploads to process.
- A few more checks for error conditions were added and messages to reflect that status were added. We now mark all failed as completed so that they will get reaped at the end of the process.
- With the additional `try`/`catch` block and checks added, the worker processes are now "cleaning up" the upload table correct when they finish, which, again, hopefully will mean we can simplify this task even more in the future.

Methods for emitting errors were added to simplify emitting those messages.
  • Loading branch information
@nvahalik
nvahalik committed Jul 18, 2024
1 parent 37c132c commit 662df38
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 109 deletions.
3 changes: 3 additions & 0 deletions src/android/PendingUploadDao.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ public interface PendingUploadDao {
@Query("SELECT COUNT(*) FROM pending_upload WHERE state = 'UPLOADED'")
int getCompletedUploadsCount();

@Query("UPDATE pending_upload SET state = 'PROCESSING' WHERE ID = :id")
void markAsProcessing(final String id);

@Query("UPDATE pending_upload SET state = 'PENDING' WHERE ID = :id")
void markAsPending(final String id);

Expand Down
255 changes: 146 additions & 109 deletions src/android/UploadTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,10 @@ public UploadTask(@NonNull Context context, @NonNull WorkerParameters workerPara

super(context, workerParams);

// Use the first upload in the database to configure everything.
nextPendingUpload = AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().getFirstPendingEntry();

// Initialize the httpClient if we don't already have it.
if (httpClient == null) {
httpClient = new OkHttpClient.Builder()
.followRedirects(true)
Expand Down Expand Up @@ -132,141 +134,154 @@ public UploadTask(@NonNull Context context, @NonNull WorkerParameters workerPara
@NonNull
@Override
public Result doWork() {
if(!hasNetworkConnection()) {
// If we don't have a connection, mark this job as can be retried based off of the backoff config.
// It will be picked up again later.
if (!hasNetworkConnection()) {
return Result.retry();
}

do {
nextPendingUpload = AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().getFirstPendingEntry();
try {
// Try to get the next task to work on.
nextPendingUpload = AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().getFirstPendingEntry();
// There is no task, so we have nothing to do. OK, just get out of here.
if (nextPendingUpload == null) {
return Result.success();
}

final String id = nextPendingUpload.getInputData().getString(KEY_INPUT_ID);
// Update the Pending Upload DB to indicate that our upload is processing. This means that the next uploads will be able to continue.
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsProcessing(nextPendingUpload.getId());

if (id == null) {
FileTransferBackground.logMessageError("doWork: ID is invalid !", null);
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsPending(nextPendingUpload.getId());
return Result.failure();
}
final String id = nextPendingUpload.getInputData().getString(KEY_INPUT_ID);

// Check retry count
if (getRunAttemptCount() > MAX_TRIES) {
return Result.success(new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, true)
.putString(KEY_OUTPUT_FAILURE_REASON, "Too many retries")
.putBoolean(KEY_OUTPUT_FAILURE_CANCELED, false)
.build()
);
}
// If the ID is null... why would it be null? Then mark the UploadTask as failed and get rid of it.
if (id == null) {
FileTransferBackground.logMessageError("doWork: ID is invalid !", null);
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
}

Request request = null;
try {
request = createRequest();
} catch (FileNotFoundException e) {
FileTransferBackground.logMessageError("doWork: File not found!", e);
final Data data = new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, true)
.putString(KEY_OUTPUT_FAILURE_REASON, "File not found")
.putBoolean(KEY_OUTPUT_FAILURE_CANCELED, false)
.build();
// Mark it as done.
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
AckDatabase.getInstance(getApplicationContext()).uploadEventDao().insert(new UploadEvent(id, data));
return Result.success(data);
} catch (NullPointerException e) {
return Result.retry();
}
// Check retry count
if (getRunAttemptCount() > MAX_TRIES) {
return Result.success(new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, true)
.putString(KEY_OUTPUT_FAILURE_REASON, "Too many retries")
.putBoolean(KEY_OUTPUT_FAILURE_CANCELED, false)
.build()
);
}

// Register me
uploadForegroundNotification.progress(getId(), 0f);
handleNotification();
Request request = null;
try {
request = createRequest();
} catch (FileNotFoundException e) {
FileTransferBackground.logMessageError("doWork: File not found!", e);

// Start call
currentCall = httpClient.newCall(request);
// Emit an error. We can't recover so mark it as done!
this.emitUploadError(id, "File not found");
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
} catch (NullPointerException e) {
// No clue. Get out of here.
this.emitUploadError(id, "Null pointer exception");
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
}

// Block until call is finished (or cancelled)
Response response = null;
try {
if (!DEBUG_SKIP_UPLOAD) {
try {
// Register me
uploadForegroundNotification.progress(getId(), 0f);
handleNotification();

// Start call
currentCall = httpClient.newCall(request);

// Block until call is finished (or cancelled)
Response response = null;
try {
if (!DEBUG_SKIP_UPLOAD) {
try {
response = currentCall.execute();
} catch (SocketTimeoutException e) {
try {
response = currentCall.execute();
} catch (SocketTimeoutException e) {
return Result.retry();
}
} catch (SocketException | ProtocolException | SSLException e) {
currentCall.cancel();
return Result.retry();
}
} catch (SocketException | ProtocolException | SSLException e) {
currentCall.cancel();
return Result.retry();
}
} else {
for (int i = 0; i < 10; i++) {
handleProgress(i * 100, 1000);
// Can be interrupted
Thread.sleep(200);
if (isStopped()) {
throw new InterruptedException("Stopped");
} else {
for (int i = 0; i < 10; i++) {
handleProgress(i * 100, 1000);
// Can be interrupted
Thread.sleep(200);
if (isStopped()) {
throw new InterruptedException("Stopped");
}
}
}
} catch (IOException | InterruptedException e) {
// If it was user cancelled its ok
// See #handleProgress for cancel code
if (isStopped()) {
Data data = this.emitUploadError(id, "User cancelled", true);
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
return Result.success(data);
} else {
// But if it was not it must be a connectivity problem or
// something similar so we retry later
FileTransferBackground.logMessageError("doWork: Call failed, retrying later", e);

this.emitUploadError(id, "System error: " + e.getMessage());
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
continue;
}
} finally {
// Always remove ourselves from the notification
uploadForegroundNotification.done(getId());
}
} catch (IOException | InterruptedException e) {
// If it was user cancelled its ok
// See #handleProgress for cancel code
if (isStopped()) {
final Data data = new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, true)
.putString(KEY_OUTPUT_FAILURE_REASON, "User cancelled")
.putBoolean(KEY_OUTPUT_FAILURE_CANCELED, true)
.build();

if (response == null) {
this.emitUploadError(id, "Upload response was null");
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
AckDatabase.getInstance(getApplicationContext()).uploadEventDao().insert(new UploadEvent(id, data));
return Result.success(data);
} else {
// But if it was not it must be a connectivity problem or
// something similar so we retry later
FileTransferBackground.logMessageError("doWork: Call failed, retrying later", e);
return Result.retry();
continue;
}
} finally {
// Always remove ourselves from the notification
uploadForegroundNotification.done(getId());
}

// Start building the output data
final Data.Builder outputData = new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, false)
.putInt(KEY_OUTPUT_STATUS_CODE, (!DEBUG_SKIP_UPLOAD) ? response.code() : 200);

// Try read the response body, if any
try {
final String res;
if (!DEBUG_SKIP_UPLOAD) {
res = response.body() != null ? response.body().string() : "";
} else {
res = "<span>heyo</span>";
}
final String filename = "upload-response-" + getId() + ".cached-response";
// Start building the output data
final Data.Builder outputData = new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, false)
.putInt(KEY_OUTPUT_STATUS_CODE, (!DEBUG_SKIP_UPLOAD) ? response.code() : 200);

// Try read the response body, if any
try {
final String res;
if (!DEBUG_SKIP_UPLOAD) {
res = response.body() != null ? response.body().string() : "";
} else {
res = "<span>heyo</span>";
}
final String filename = "upload-response-" + getId() + ".cached-response";

try (FileOutputStream fos = getApplicationContext().openFileOutput(filename, Context.MODE_PRIVATE)) {
fos.write(res.getBytes(StandardCharsets.UTF_8));
}
try (FileOutputStream fos = getApplicationContext().openFileOutput(filename, Context.MODE_PRIVATE)) {
fos.write(res.getBytes(StandardCharsets.UTF_8));
}

outputData.putString(KEY_OUTPUT_RESPONSE_FILE, filename);
outputData.putString(KEY_OUTPUT_RESPONSE_FILE, filename);
} catch (IOException e) {
// Should never happen, but if it does it has something to do with reading the response
FileTransferBackground.logMessageError("doWork: Error while reading the response body", e);

} catch (IOException e) {
// Should never happen, but if it does it has something to do with reading the response
FileTransferBackground.logMessageError("doWork: Error while reading the response body", e);
// But recover and replace the body with something else
outputData.putString(KEY_OUTPUT_RESPONSE_FILE, null);
}

// But recover and replace the body with something else
outputData.putString(KEY_OUTPUT_RESPONSE_FILE, null);
final Data data = outputData.build();
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
AckDatabase.getInstance(getApplicationContext()).uploadEventDao().insert(new UploadEvent(id, data));
} catch (Exception e) {
this.emitUploadError(nextPendingUpload.getId(), "Caught major exception: " + e.getMessage());
}
} while (AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().getPendingUploadsCount() > 0);

final Data data = outputData.build();
AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().markAsUploaded(nextPendingUpload.getId());
AckDatabase.getInstance(getApplicationContext()).uploadEventDao().insert(new UploadEvent(id, data));
} while(AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().getPendingUploadsCount() > 0);

// Now that we've exited the loop, we don't want to have the database continue to grow.
final List<PendingUpload> pendingUploads = AckDatabase.getInstance(getApplicationContext()).pendingUploadDao().getCompletedUploads();

for (PendingUpload pendingUpload: pendingUploads) {
Expand Down Expand Up @@ -303,6 +318,28 @@ private void handleProgress(long bytesWritten, long totalBytes) {
handleNotification();
}


private Data emitUploadError(String id, String reason) {
return this.emitUploadError(id, reason, false);
}

private Data emitUploadError(String id, String reason, boolean wasCancelled) {
return this.emitUploadData(id, reason, true, wasCancelled);
}

private Data emitUploadData(String id, String reason, boolean isError, boolean wasCancelled) {
final Data data = new Data.Builder()
.putString(KEY_OUTPUT_ID, id)
.putBoolean(KEY_OUTPUT_IS_ERROR, isError)
.putString(KEY_OUTPUT_FAILURE_REASON, reason)
.putBoolean(KEY_OUTPUT_FAILURE_CANCELED, wasCancelled)
.build();

AckDatabase.getInstance(getApplicationContext()).uploadEventDao().insert(new UploadEvent(id, data));

return data;
}

/**
* Create the OkHttp request that will be used, already filled with input data.
*
Expand Down

0 comments on commit 662df38

Please sign in to comment.