-
Notifications
You must be signed in to change notification settings - Fork 1.3k
5128: Fix stuck uploads #5257
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
5128: Fix stuck uploads #5257
Changes from 16 commits
4aa4ea0
96058ff
870f286
1ebb117
2dd2be1
6b9e5ab
2ec4667
f1b2e03
0a64989
c785d6f
e1855b9
6d013e6
4cafd2a
04b7255
533e976
4fbe261
c35d936
d1bfb48
3ff04d3
ab2f114
faf0cde
f441eaa
5ec4d56
034d8d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
nicolas-raoul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@BindView(R.id.card_view_nearby) public NearbyNotificationCardView nearbyNotificationCardView; | ||
@BindView(R.id.campaigns_view) CampaignView campaignView; | ||
|
@@ -593,6 +594,15 @@ public void notifyDataSetChanged() { | |
} | ||
} | ||
|
||
/** | ||
* 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,22 @@ public void notifyDataSetChanged() { | |
@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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to test the change in my device. Blindly retrying all failed uploads does seem to be affecting the UX badly. The app seems to just retrying genuinely failed uploads again-and-again each time I open the app. Is there really no other attribute that we have that could help with identifying if an upload failed for a genuine reason? (I'm presuming its because of beta cluster not supporting stashed uploads). Also, I remember cancelling all uploads manually one time. The cancel didn't seem to have worked. Has anyone else observed this too? |
||
int retries = contribution.getRetries(); | ||
/* 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 %d times", retries + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can it be changed to "Retried upload %d times" so it's clear what we're trying to retry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed the debug log line could say that, and also say what file, because often several files are being retried at the same time. |
||
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()); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
|
@@ -17,8 +18,12 @@ | |
import androidx.appcompat.widget.Toolbar; | ||
import androidx.fragment.app.Fragment; | ||
import androidx.fragment.app.FragmentManager; | ||
import androidx.work.BackoffPolicy; | ||
import androidx.work.Constraints; | ||
import androidx.work.ExistingWorkPolicy; | ||
import androidx.work.NetworkType; | ||
import androidx.work.OneTimeWorkRequest; | ||
import androidx.work.OutOfQuotaPolicy; | ||
import androidx.work.WorkManager; | ||
import butterknife.BindView; | ||
import butterknife.ButterKnife; | ||
|
@@ -47,6 +52,9 @@ | |
import fr.free.nrw.commons.upload.worker.UploadWorker; | ||
import fr.free.nrw.commons.utils.PermissionUtils; | ||
import fr.free.nrw.commons.utils.ViewUtilWrapper; | ||
import io.reactivex.schedulers.Schedulers; | ||
import java.util.Collections; | ||
import java.util.concurrent.TimeUnit; | ||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import timber.log.Timber; | ||
|
@@ -58,6 +66,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 +148,9 @@ public void onCreate(Bundle savedInstanceState) { | |
setTitle(getString(R.string.navigation_item_explore)); | ||
setUpLoggedOutPager(); | ||
} else { | ||
if (applicationKvStore.getBoolean("firstrun", true)) { | ||
applicationKvStore.putBoolean("firstBigUploadSet", true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate the consistency with |
||
} | ||
if(savedInstanceState == null){ | ||
//starting a fresh fragment. | ||
// Open Last opened screen if it is Contributions or Nearby, otherwise Contributions | ||
|
@@ -360,6 +373,21 @@ public boolean onOptionsItemSelected(MenuItem item) { | |
} | ||
} | ||
|
||
/** | ||
* 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,9 +397,18 @@ public void toggleLimitedConnectionMode() { | |
viewUtilWrapper | ||
.showShortToast(getBaseContext(), getString(R.string.limited_connection_enabled)); | ||
} else { | ||
Constraints constraints = new Constraints.Builder() | ||
.setRequiredNetworkType(NetworkType.CONNECTED) | ||
.build(); | ||
OneTimeWorkRequest restartUploadsRequest = new OneTimeWorkRequest | ||
.Builder(UploadWorker.class) | ||
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) | ||
.setConstraints(constraints) | ||
.setBackoffCriteria(BackoffPolicy.LINEAR, 10, TimeUnit.SECONDS) | ||
.build(); | ||
WorkManager.getInstance(getApplicationContext()).enqueueUniqueWork( | ||
UploadWorker.class.getSimpleName(), | ||
ExistingWorkPolicy.APPEND_OR_REPLACE, OneTimeWorkRequest.from(UploadWorker.class)); | ||
ExistingWorkPolicy.APPEND_OR_REPLACE, restartUploadsRequest); | ||
|
||
viewUtilWrapper | ||
.showShortToast(getBaseContext(), getString(R.string.limited_connection_disabled)); | ||
|
@@ -405,6 +442,8 @@ protected void onResume() { | |
(!applicationKvStore.getBoolean("login_skipped"))) { | ||
WelcomeActivity.startYourself(this); | ||
} | ||
|
||
retryAllFailedUploads(); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,20 @@ | ||
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; | ||
import android.annotation.SuppressLint; | ||
import android.app.ProgressDialog; | ||
import android.content.Intent; | ||
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; | ||
|
@@ -24,8 +23,12 @@ | |
import androidx.recyclerview.widget.RecyclerView; | ||
import androidx.viewpager.widget.PagerAdapter; | ||
import androidx.viewpager.widget.ViewPager; | ||
import androidx.work.BackoffPolicy; | ||
import androidx.work.Constraints; | ||
import androidx.work.ExistingWorkPolicy; | ||
import androidx.work.NetworkType; | ||
import androidx.work.OneTimeWorkRequest; | ||
import androidx.work.OutOfQuotaPolicy; | ||
import androidx.work.WorkManager; | ||
import butterknife.BindView; | ||
import butterknife.ButterKnife; | ||
|
@@ -35,7 +38,6 @@ | |
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.mwapi.UserClient; | ||
|
@@ -57,6 +59,7 @@ | |
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.concurrent.TimeUnit; | ||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import timber.log.Timber; | ||
|
@@ -317,9 +320,18 @@ public void updateTopCardTitle() { | |
|
||
@Override | ||
public void makeUploadRequest() { | ||
Constraints constraints = new Constraints.Builder() | ||
.setRequiredNetworkType(NetworkType.CONNECTED) | ||
.build(); | ||
OneTimeWorkRequest uploadRequest = new OneTimeWorkRequest | ||
.Builder(UploadWorker.class) | ||
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) | ||
.setConstraints(constraints) | ||
.setBackoffCriteria(BackoffPolicy.LINEAR, 10, TimeUnit.SECONDS) | ||
.build(); | ||
WorkManager.getInstance(getApplicationContext()).enqueueUniqueWork( | ||
UploadWorker.class.getSimpleName(), | ||
ExistingWorkPolicy.APPEND_OR_REPLACE, OneTimeWorkRequest.from(UploadWorker.class)); | ||
ExistingWorkPolicy.APPEND_OR_REPLACE, uploadRequest); | ||
} | ||
|
||
@Override | ||
|
@@ -364,6 +376,24 @@ private void receiveSharedItems() { | |
.getQuantityString(R.plurals.upload_count_title, uploadableFiles.size(), uploadableFiles.size())); | ||
|
||
fragments = new ArrayList<>(); | ||
// Suggest users to switch to Unrestricted battery usage mode | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Append |
||
if (uploadableFiles.size() > 3 | ||
&& defaultKvStore.getBoolean("firstBigUploadSet")) { | ||
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), | ||
() -> { | ||
Intent batteryOptimisationSettingsIntent = new Intent( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment about the library you tried, explaining how it did not make the setting dialog appear on Pixel nor Xiaomi. Someone may want to revisit that in the future. |
||
Settings.ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS); | ||
startActivity(batteryOptimisationSettingsIntent); | ||
}, | ||
() -> {} | ||
); | ||
defaultKvStore.putBoolean("firstBigUploadSet", false); | ||
} | ||
for (UploadableFile uploadableFile : uploadableFiles) { | ||
UploadMediaDetailFragment uploadMediaDetailFragment = new UploadMediaDetailFragment(); | ||
uploadMediaDetailFragment.setImageTobeUploaded(uploadableFile, place); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a schema change to the contribution table and requires a bump of the db version number [ref]. Kindly take care of the same.