5136: Fix retried uploads stuck in queued state (#5272)

* fix stuck uploads

* automate retries for failed uploads once the user returns to the app

* UploadWorker: modify PendingIntent flag and Android version code

* MainActivity: remove automatic retry logic

* Revert "MainActivity: remove automatic retry logic"

* set work request as expedited

* handle notification for foreground service on older versions of Android

* set backoff criteria for work requests

* enqueue failed uploads for a retry

* revert "enqueue failed uploads for a retry"

* limit the number of retries for a failed upload

* add a popup that suggests users to switch to unrestricted battery usage mode

* take users to the battery settings page on the first big upload

* take users to battery optimisation settings page using the standard intent

* add instructions to the battery optimisation settings popup

* remove the first usage of fr.free.nrw.commons from the popup

* comply with the wording in the OS settings

* modify battery optimisation popup instructions, add comments and rename firstBigUploadSet

* add filename to the retry log statement

* update database version

* make battery optimisation dialog appear only on Android 6 and above

* use foreground service instead of setting work request as expedited

* fix retried uploads stuck in queued state

* use MIN_BACKOFF_MILLIS constant instead of using the number 10 and add comments

* factorise the creation of the new OneTimeWorkRequest at one place

* ensure work requests are in accordance with the unit tests

* forbid retries for images which have got uploaded without caption

* add a TODO for the suggestion related to retries

* revert "forbid retries for images which have got uploaded without caption"
This commit is contained in:
Ritika Pahwa 2023-09-09 22:46:13 +05:30 committed by GitHub
parent 4540f54d59
commit 81030d1e78
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 200 additions and 47 deletions

View file

@ -142,7 +142,7 @@ dependencies {
implementation "androidx.multidex:multidex:$MULTIDEX_VERSION"
def work_version = "2.8.0"
def work_version = "2.8.1"
// Kotlin + coroutines
implementation "androidx.work:work-runtime-ktx:$work_version"
implementation("androidx.work:work-runtime:$work_version")
@ -168,7 +168,7 @@ project.gradle.taskGraph.whenReady {
}
android {
compileSdkVersion 31
compileSdkVersion 33
defaultConfig {
//applicationId 'fr.free.nrw.commons'

View file

@ -43,7 +43,11 @@ data class Contribution constructor(
var hasInvalidLocation : Int = 0,
var contentUri: Uri? = null,
var countryCode : String? = null,
var imageSHA1 : String? = null
var imageSHA1 : String? = null,
/**
* Number of times a contribution has been retried after a failure
*/
var retries: Int = 0
) : Parcelable {
fun completeWith(media: Media): Contribution {

View file

@ -92,6 +92,7 @@ public class ContributionsFragment
private static final String CONTRIBUTION_LIST_FRAGMENT_TAG = "ContributionListFragmentTag";
private MediaDetailPagerFragment mediaDetailPagerFragment;
static final String MEDIA_DETAIL_PAGER_FRAGMENT_TAG = "MediaDetailFragmentTag";
private static final int MAX_RETRIES = 10;
@BindView(R.id.card_view_nearby) public NearbyNotificationCardView nearbyNotificationCardView;
@BindView(R.id.campaigns_view) CampaignView campaignView;
@ -593,6 +594,15 @@ public class ContributionsFragment
}
}
/**
* Restarts the upload process for a contribution
* @param contribution
*/
public void restartUpload(Contribution contribution) {
contribution.setState(Contribution.STATE_QUEUED);
contributionsPresenter.saveContribution(contribution);
Timber.d("Restarting for %s", contribution.toString());
}
/**
* Retry upload when it is failed
*
@ -601,10 +611,23 @@ public class ContributionsFragment
@Override
public void retryUpload(Contribution contribution) {
if (NetworkUtils.isInternetConnectionEstablished(getContext())) {
if (contribution.getState() == STATE_FAILED || contribution.getState() == STATE_PAUSED || contribution.getState()==Contribution.STATE_QUEUED_LIMITED_CONNECTION_MODE) {
contribution.setState(Contribution.STATE_QUEUED);
contributionsPresenter.saveContribution(contribution);
Timber.d("Restarting for %s", contribution.toString());
if (contribution.getState() == STATE_PAUSED || contribution.getState()==Contribution.STATE_QUEUED_LIMITED_CONNECTION_MODE) {
restartUpload(contribution);
} else if (contribution.getState() == STATE_FAILED) {
int retries = contribution.getRetries();
// TODO: Improve UX. Additional details: https://github.com/commons-app/apps-android-commons/pull/5257#discussion_r1304662562
/* 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) {
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
Toast.makeText(getContext(),
R.string.retry_limit_reached, Toast.LENGTH_SHORT).show();
}
} else {
Timber.d("Skipping re-upload for non-failed %s", contribution.toString());
}

View file

@ -1,18 +1,12 @@
package fr.free.nrw.commons.contributions;
import androidx.work.ExistingWorkPolicy;
import androidx.work.OneTimeWorkRequest;
import androidx.work.WorkManager;
import fr.free.nrw.commons.MediaDataExtractor;
import fr.free.nrw.commons.contributions.ContributionsContract.UserActionListener;
import fr.free.nrw.commons.di.CommonsApplicationModule;
import fr.free.nrw.commons.upload.worker.UploadWorker;
import fr.free.nrw.commons.upload.worker.WorkRequestHelper;
import io.reactivex.Scheduler;
import io.reactivex.disposables.CompositeDisposable;
import io.reactivex.functions.Action;
import io.reactivex.functions.Consumer;
import java.util.Collections;
import java.util.List;
import javax.inject.Inject;
import javax.inject.Named;
@ -76,11 +70,7 @@ public class ContributionsPresenter implements UserActionListener {
compositeDisposable.add(repository
.save(contribution)
.subscribeOn(ioThreadScheduler)
.subscribe(() -> {
WorkManager.getInstance(view.getContext().getApplicationContext())
.enqueueUniqueWork(
UploadWorker.class.getSimpleName(),
ExistingWorkPolicy.KEEP, OneTimeWorkRequest.from(UploadWorker.class));
}));
.subscribe(() -> WorkRequestHelper.Companion.makeOneTimeWorkRequest(
view.getContext().getApplicationContext(), ExistingWorkPolicy.KEEP)));
}
}

View file

@ -1,6 +1,7 @@
package fr.free.nrw.commons.contributions;
import android.Manifest.permission;
import android.annotation.SuppressLint;
import android.app.Activity;
import android.content.Context;
import android.content.Intent;
@ -18,8 +19,6 @@ import androidx.appcompat.widget.Toolbar;
import androidx.fragment.app.Fragment;
import androidx.fragment.app.FragmentManager;
import androidx.work.ExistingWorkPolicy;
import androidx.work.OneTimeWorkRequest;
import androidx.work.WorkManager;
import butterknife.BindView;
import butterknife.ButterKnife;
import fr.free.nrw.commons.CommonsApplication;
@ -44,9 +43,11 @@ import fr.free.nrw.commons.notification.NotificationController;
import fr.free.nrw.commons.quiz.QuizChecker;
import fr.free.nrw.commons.settings.SettingsFragment;
import fr.free.nrw.commons.theme.BaseActivity;
import fr.free.nrw.commons.upload.worker.UploadWorker;
import fr.free.nrw.commons.upload.worker.WorkRequestHelper;
import fr.free.nrw.commons.utils.PermissionUtils;
import fr.free.nrw.commons.utils.ViewUtilWrapper;
import io.reactivex.schedulers.Schedulers;
import java.util.Collections;
import javax.inject.Inject;
import javax.inject.Named;
import timber.log.Timber;
@ -58,6 +59,8 @@ public class MainActivity extends BaseActivity
SessionManager sessionManager;
@Inject
ContributionController controller;
@Inject
ContributionDao contributionDao;
@BindView(R.id.toolbar)
Toolbar toolbar;
@BindView(R.id.pager)
@ -138,6 +141,9 @@ public class MainActivity extends BaseActivity
setTitle(getString(R.string.navigation_item_explore));
setUpLoggedOutPager();
} else {
if (applicationKvStore.getBoolean("firstrun", true)) {
applicationKvStore.putBoolean("hasAlreadyLaunchedBigMultiupload", false);
}
if(savedInstanceState == null){
//starting a fresh fragment.
// Open Last opened screen if it is Contributions or Nearby, otherwise Contributions
@ -360,6 +366,21 @@ public class MainActivity extends BaseActivity
}
}
/**
* Retry all failed uploads as soon as the user returns to the app
*/
@SuppressLint("CheckResult")
private void retryAllFailedUploads() {
contributionDao.
getContribution(Collections.singletonList(Contribution.STATE_FAILED))
.subscribeOn(Schedulers.io())
.subscribe(failedUploads -> {
for (Contribution contribution: failedUploads) {
contributionsFragment.retryUpload(contribution);
}
});
}
public void toggleLimitedConnectionMode() {
defaultKvStore.putBoolean(CommonsApplication.IS_LIMITED_CONNECTION_MODE_ENABLED,
!defaultKvStore
@ -369,10 +390,8 @@ public class MainActivity extends BaseActivity
viewUtilWrapper
.showShortToast(getBaseContext(), getString(R.string.limited_connection_enabled));
} else {
WorkManager.getInstance(getApplicationContext()).enqueueUniqueWork(
UploadWorker.class.getSimpleName(),
ExistingWorkPolicy.APPEND_OR_REPLACE, OneTimeWorkRequest.from(UploadWorker.class));
WorkRequestHelper.Companion.makeOneTimeWorkRequest(getApplicationContext(),
ExistingWorkPolicy.APPEND_OR_REPLACE);
viewUtilWrapper
.showShortToast(getBaseContext(), getString(R.string.limited_connection_disabled));
}
@ -406,6 +425,8 @@ public class MainActivity extends BaseActivity
defaultKvStore.putBoolean("inAppCameraFirstRun", true);
WelcomeActivity.startYourself(this);
}
retryAllFailedUploads();
}
@Override

View file

@ -16,7 +16,7 @@ open class OnSwipeTouchListener(context: Context?) : View.OnTouchListener {
private val SWIPE_THRESHOLD_WIDTH = (getScreenResolution(context!!)).first / 3
private val SWIPE_VELOCITY_THRESHOLD = 1000
override fun onTouch(view: View?, motionEvent: MotionEvent?): Boolean {
override fun onTouch(view: View?, motionEvent: MotionEvent): Boolean {
return gestureDetector.onTouchEvent(motionEvent)
}
@ -32,7 +32,7 @@ open class OnSwipeTouchListener(context: Context?) : View.OnTouchListener {
inner class GestureListener : GestureDetector.SimpleOnGestureListener() {
override fun onDown(e: MotionEvent?): Boolean {
override fun onDown(e: MotionEvent): Boolean {
return true
}

View file

@ -15,7 +15,7 @@ import fr.free.nrw.commons.upload.depicts.DepictsDao
* The database for accessing the respective DAOs
*
*/
@Database(entities = [Contribution::class, Depicts::class, UploadedStatus::class, NotForUploadStatus::class, ReviewEntity::class], version = 15, exportSchema = false)
@Database(entities = [Contribution::class, Depicts::class, UploadedStatus::class, NotForUploadStatus::class, ReviewEntity::class], version = 16, exportSchema = false)
@TypeConverters(Converters::class)
abstract class AppDatabase : RoomDatabase() {
abstract fun contributionDao(): ContributionDao

View file

@ -1,7 +1,6 @@
package fr.free.nrw.commons.upload;
import static fr.free.nrw.commons.contributions.ContributionController.ACTION_INTERNAL_UPLOADS;
import static fr.free.nrw.commons.upload.UploadPresenter.COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES;
import static fr.free.nrw.commons.wikidata.WikidataConstants.PLACE_OBJECT;
import android.Manifest;
@ -10,14 +9,15 @@ import android.app.ProgressDialog;
import android.content.Intent;
import android.location.Location;
import android.location.LocationManager;
import android.os.Build;
import android.os.Bundle;
import android.provider.Settings;
import android.util.DisplayMetrics;
import android.view.View;
import android.widget.ImageButton;
import android.widget.LinearLayout;
import android.widget.RelativeLayout;
import android.widget.TextView;
import androidx.appcompat.app.AlertDialog;
import androidx.cardview.widget.CardView;
import androidx.fragment.app.Fragment;
import androidx.fragment.app.FragmentManager;
@ -27,8 +27,6 @@ import androidx.recyclerview.widget.RecyclerView;
import androidx.viewpager.widget.PagerAdapter;
import androidx.viewpager.widget.ViewPager;
import androidx.work.ExistingWorkPolicy;
import androidx.work.OneTimeWorkRequest;
import androidx.work.WorkManager;
import butterknife.BindView;
import butterknife.ButterKnife;
import butterknife.OnClick;
@ -37,7 +35,6 @@ import fr.free.nrw.commons.R;
import fr.free.nrw.commons.auth.LoginActivity;
import fr.free.nrw.commons.auth.SessionManager;
import fr.free.nrw.commons.contributions.ContributionController;
import fr.free.nrw.commons.contributions.MainActivity;
import fr.free.nrw.commons.filepicker.UploadableFile;
import fr.free.nrw.commons.kvstore.JsonKvStore;
import fr.free.nrw.commons.location.LatLng;
@ -52,7 +49,7 @@ import fr.free.nrw.commons.upload.depicts.DepictsFragment;
import fr.free.nrw.commons.upload.license.MediaLicenseFragment;
import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment;
import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment.UploadMediaDetailFragmentCallback;
import fr.free.nrw.commons.upload.worker.UploadWorker;
import fr.free.nrw.commons.upload.worker.WorkRequestHelper;
import fr.free.nrw.commons.utils.DialogUtil;
import fr.free.nrw.commons.utils.PermissionUtils;
import fr.free.nrw.commons.utils.ViewUtil;
@ -336,9 +333,8 @@ public class UploadActivity extends BaseActivity implements UploadContract.View,
@Override
public void makeUploadRequest() {
WorkManager.getInstance(getApplicationContext()).enqueueUniqueWork(
UploadWorker.class.getSimpleName(),
ExistingWorkPolicy.APPEND_OR_REPLACE, OneTimeWorkRequest.from(UploadWorker.class));
WorkRequestHelper.Companion.makeOneTimeWorkRequest(getApplicationContext(),
ExistingWorkPolicy.APPEND_OR_REPLACE);
}
@Override
@ -383,6 +379,42 @@ public class UploadActivity extends BaseActivity implements UploadContract.View,
.getQuantityString(R.plurals.upload_count_title, uploadableFiles.size(), uploadableFiles.size()));
fragments = new ArrayList<>();
/* Suggest users to turn battery optimisation off when uploading more than a few files.
That's because we have noticed that many-files uploads have
a much higher probability of failing than uploads with less files.
Show the dialog for Android 6 and above as
the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent was added in API level 23
*/
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
if (uploadableFiles.size() > 3
&& !defaultKvStore.getBoolean("hasAlreadyLaunchedBigMultiupload")) {
DialogUtil.showAlertDialog(
this,
getString(R.string.unrestricted_battery_mode),
getString(R.string.suggest_unrestricted_mode),
getString(R.string.title_activity_settings),
getString(R.string.cancel),
() -> {
/* Since opening the right settings page might be device dependent, using
https://github.com/WaseemSabir/BatteryPermissionHelper
directly appeared like a promising idea.
However, this simply closed the popup and did not make
the settings page appear on a Pixel as well as a Xiaomi device.
Used the standard intent instead of using this library as
it shows a list of all the apps on the device and allows users to
turn battery optimisation off.
*/
Intent batteryOptimisationSettingsIntent = new Intent(
Settings.ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS);
startActivity(batteryOptimisationSettingsIntent);
},
() -> {}
);
defaultKvStore.putBoolean("hasAlreadyLaunchedBigMultiupload", true);
}
}
for (UploadableFile uploadableFile : uploadableFiles) {
UploadMediaDetailFragment uploadMediaDetailFragment = new UploadMediaDetailFragment();

View file

@ -1,17 +1,20 @@
package fr.free.nrw.commons.upload.worker
import android.annotation.SuppressLint
import android.app.Notification
import android.app.PendingIntent
import android.app.TaskStackBuilder
import android.content.Context
import android.content.Intent
import android.graphics.BitmapFactory
import android.os.Build
import androidx.core.app.NotificationCompat
import androidx.core.app.NotificationManagerCompat
import androidx.work.CoroutineWorker
import androidx.work.Data
import androidx.work.WorkerParameters
import androidx.multidex.BuildConfig
import androidx.work.ForegroundInfo
import dagger.android.ContributesAndroidInjector
import fr.free.nrw.commons.CommonsApplication
import fr.free.nrw.commons.Media
@ -40,6 +43,7 @@ import kotlinx.coroutines.flow.map
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import timber.log.Timber
import java.net.SocketTimeoutException
import java.util.*
import java.util.regex.Pattern
import javax.inject.Inject
@ -161,6 +165,8 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
override suspend fun doWork(): Result {
var countUpload = 0
// Start a foreground service
setForeground(createForegroundInfo())
notificationManager = NotificationManagerCompat.from(appContext)
val processingUploads = getNotificationBuilder(
CommonsApplication.NOTIFICATION_CHANNEL_ID_ALL
@ -170,15 +176,15 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
.blockingGet()
//Showing initial notification for the number of uploads being processed
Timber.e("Queued Contributions: "+ queuedContributions.size)
Timber.e("Queued Contributions: " + queuedContributions.size)
processingUploads.setContentTitle(appContext.getString(R.string.starting_uploads))
processingUploads.setContentText(
appContext.resources.getQuantityString(
R.plurals.starting_multiple_uploads,
queuedContributions.size,
queuedContributions.size
)
R.plurals.starting_multiple_uploads,
queuedContributions.size,
queuedContributions.size
)
)
notificationManager?.notify(
PROCESSING_UPLOADS_NOTIFICATION_TAG,
@ -191,7 +197,7 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
so that the next one does not process these contribution again
*/
queuedContributions.forEach {
it.state=Contribution.STATE_IN_PROGRESS
it.state = Contribution.STATE_IN_PROGRESS
contributionDao.saveSynchronous(it)
}
@ -223,10 +229,37 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
PROCESSING_UPLOADS_NOTIFICATION_ID
)
}
//TODO make this smart, think of handling retries in the future
// Trigger WorkManager to process any new contributions that may have been added to the queue
val updatedContributionQueue = withContext(Dispatchers.IO) {
contributionDao.getContribution(statesToProcess).blockingGet()
}
if (updatedContributionQueue.isNotEmpty()) {
return Result.retry()
}
return Result.success()
}
/**
* Create new notification for foreground service
*/
private fun createForegroundInfo(): ForegroundInfo {
return ForegroundInfo(
1,
createNotificationForForegroundService()
)
}
override suspend fun getForegroundInfo(): ForegroundInfo {
return createForegroundInfo()
}
private fun createNotificationForForegroundService(): Notification {
// TODO: Improve notification for foreground service
return getNotificationBuilder(
CommonsApplication.NOTIFICATION_CHANNEL_ID_ALL)!!
.setContentTitle(appContext.getString(R.string.upload_in_progress))
.build()
}
/**
* Returns true is the limited connection mode is enabled
*/
@ -540,7 +573,12 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
val intent = Intent(appContext,toClass)
return TaskStackBuilder.create(appContext).run {
addNextIntentWithParentStack(intent)
getPendingIntent(0, PendingIntent.FLAG_UPDATE_CURRENT)
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
getPendingIntent(0,
PendingIntent.FLAG_IMMUTABLE or PendingIntent.FLAG_UPDATE_CURRENT)
} else {
getPendingIntent(0, PendingIntent.FLAG_UPDATE_CURRENT)
}
};
}

View file

@ -0,0 +1,41 @@
package fr.free.nrw.commons.upload.worker
import android.content.Context
import androidx.work.*
import androidx.work.WorkRequest.Companion.MIN_BACKOFF_MILLIS
import java.util.concurrent.TimeUnit
/**
* Helper class for all the one time work requests
*/
class WorkRequestHelper {
companion object {
fun makeOneTimeWorkRequest(context: Context, existingWorkPolicy: ExistingWorkPolicy) {
/* Set backoff criteria for the work request
The default backoff policy is EXPONENTIAL, but while testing we found that it
too long for the uploads to finish. So, set the backoff policy as LINEAR with the
minimum backoff delay value of 10 seconds.
More details on when exactly it is retried:
https://developer.android.com/guide/background/persistent/getting-started/define-work#retries_backoff
*/
val constraints: Constraints = Constraints.Builder()
.setRequiredNetworkType(NetworkType.CONNECTED)
.build()
val uploadRequest: OneTimeWorkRequest =
OneTimeWorkRequest.Builder(UploadWorker::class.java)
.setBackoffCriteria(
BackoffPolicy.LINEAR,
MIN_BACKOFF_MILLIS,
TimeUnit.MILLISECONDS
)
.setConstraints(constraints)
.build()
WorkManager.getInstance(context).enqueueUniqueWork(
UploadWorker::class.java.simpleName, existingWorkPolicy, uploadRequest
)
}
}
}

View file

@ -62,6 +62,7 @@
<string name="bullet">&#8226;</string>
<string name="menu_settings">Settings</string>
<string name="intent_share_upload_label">Upload to Commons</string>
<string name="upload_in_progress">Upload in progress</string>
<string name="username">Username</string>
<string name="password">Password</string>
<string name="login_credential">Log in to your Commons Beta account</string>
@ -75,6 +76,9 @@
<string name="login_success">Login success!</string>
<string name="login_failed">Login failed!</string>
<string name="upload_failed">File not found. Please try another file.</string>
<string name="retry_limit_reached">Maximum retry limit reached! Please cancel the upload and try again</string>
<string name="unrestricted_battery_mode">Turn battery optimization off?</string>
<string name="suggest_unrestricted_mode">Uploading more than 3 images works more reliably when the battery optimization is turned off. Please turn battery optimization off for the Commons app from the settings for a smooth upload experience. \n\nPossible steps to turn battery optimization off:\n\nStep 1: Tap on the \'Settings\' button below.\n\nStep 2: Switch from \'Not optimized\' to \'All apps\'.\n\nStep 3: Search for \"Commons\" or \"fr.free.nrw.commons\".\n\nStep 4: Tap it and select \'Don\'t optimize\'.\n\nStep 5: Press \'Done\'.</string>
<string name="authentication_failed">Authentication failed, please login again</string>
<string name="uploading_started">Upload started!</string>
<string name="uploading_queued">Upload queued (limited connection mode enabled)</string>