Skip to content
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.

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

Issue 5284 prevent retries for genuinely failed uploads #5613

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ data class Contribution constructor(
var dateCreatedSource: String? = null,
var wikidataPlace: WikidataPlace? = null,
var chunkInfo: ChunkInfo? = null,
// add error message to keep track of error message for failed upload
var errorMessage: String? = null,
var exceptionMessage: String? = null,
/**
* @return array list of entityids for the depictions
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import fr.free.nrw.commons.notification.NotificationController;
import fr.free.nrw.commons.profile.ProfileActivity;
import fr.free.nrw.commons.theme.BaseActivity;
import java.util.Arrays;
import java.util.Date;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -116,6 +117,19 @@ public class ContributionsFragment
@Inject
SessionManager sessionManager;


private final List<String> STASH_ERROR_CODES = Arrays.asList(
"file path is null",
"upload to stash failed"
);

private final List<String> STASH_EXCEPTIONS = Arrays.asList(
"StashUploadException",
"IOException"
);



private LatLng curLatLng;

private boolean isFragmentAttachedBefore = false;
Expand Down Expand Up @@ -670,12 +684,16 @@ public void retryUpload(Contribution contribution) {
/* Limit the number of retries for a failed upload
to handle cases like invalid filename as such uploads
will never be successful */
if(retries < MAX_RETRIES) {
if (contribution.getErrorMessage() != null && isGenuineError(contribution)) {
// we have a genuine error here therefore no retry
// TODO: notify user of error

} else if(retries < MAX_RETRIES) {
contribution.setRetries(retries + 1);
Timber.d("Retried uploading %s %d times", contribution.getMedia().getFilename(), retries + 1);
restartUpload(contribution);
} else {
// TODO: Show the exact reason for failure
// TODO: notify user of error
Toast.makeText(getContext(),
R.string.retry_limit_reached, Toast.LENGTH_SHORT).show();
}
Expand All @@ -686,6 +704,27 @@ public void retryUpload(Contribution contribution) {
ViewUtil.showLongToast(getContext(), R.string.this_function_needs_network_connection);
}

}
/**
* Check if the error that occurs is a genuine error
* @param contribution contribution to be retried
*/
public boolean isGenuineError(Contribution contribution) {

//check against array for genuine errors
for (String error : STASH_ERROR_CODES) {
if (contribution.getErrorMessage().contains(error)) {
return true;
}
}

for (String error : STASH_EXCEPTIONS) {
if (contribution.getExceptionMessage().contains(error)) {
return true;
}
}
return false;

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,10 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
*/
@SuppressLint("StringFormatInvalid")
private suspend fun uploadContribution(contribution: Contribution) {
contribution.errorMessage = null
if (contribution.localUri == null || contribution.localUri.path == null) {
Timber.e("""upload: ${contribution.media.filename} failed, file path is null""")
contribution.errorMessage = "file path is null"
}

val media = contribution.media
Expand Down Expand Up @@ -378,13 +380,20 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :

} else {
Timber.e("Stash Upload failed")
if (uploadResult != null && contribution.errorMessage == null) {
contribution.errorMessage = uploadResult.result
}
showFailedNotification(contribution)
contribution.state = Contribution.STATE_FAILED
contribution.chunkInfo = null
contributionDao.save(contribution).blockingAwait()

}
}catch (exception : Exception){
if (contribution.errorMessage == null) {
contribution.errorMessage = "upload from stash failed with exception"
}
contribution.exceptionMessage = exception.toString()
Timber.e(exception)
Timber.e("Upload from stash failed for contribution : $filename")
showFailedNotification(contribution)
Expand All @@ -401,6 +410,9 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
contributionDao.saveSynchronous(contribution)
}
else -> {
if (contribution.errorMessage == null) {
contribution.errorMessage = "upload to stash failed"
}
Timber.e("""upload file to stash failed with status: ${stashUploadResult.state}""")
showFailedNotification(contribution)
contribution.state = Contribution.STATE_FAILED
Expand All @@ -409,6 +421,10 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
}
}
}catch (exception: Exception){
if (contribution.errorMessage == null) {
contribution.errorMessage = "upload to stash failed with exception"
}
contribution.exceptionMessage = exception.toString()
Timber.e(exception)
Timber.e("Stash upload failed for contribution: $filename")
showFailedNotification(contribution)
Expand Down