Pārlūkot izejas kodu

FilesUploadWorker: fix infinite loop when uploads failed

With the previous approach (uploadsStorageManager.gtCurrentAndPendingUploadsForAccount),
an upload that is, for example, waiting for wifi, would always be returned in the call,
thus the list would never be empty.

To avoid this but still process the uploads in pages to avoid OOM, we'll request them page-wise
from the StorageManager, with the pages ordered by ascending ID. This way we will only process each upload once,
but newly added uploads after we start will not be missed.

Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
Álvaro Brey 2 gadi atpakaļ
vecāks
revīzija
f425045a31

+ 12 - 10
app/src/main/java/com/nextcloud/client/jobs/FilesUploadWorker.kt

@@ -78,21 +78,24 @@ class FilesUploadWorker(
             return Result.failure() // user account is needed
         }
 
-        // get all pending uploads
-        var currentAndPendingUploadsForAccount =
-            uploadsStorageManager.getCurrentAndPendingUploadsForAccount(MAX_UPLOADS_QUERY, accountName)
-        while (currentAndPendingUploadsForAccount.isNotEmpty()) {
-            Log_OC.d(TAG, "Handling ${currentAndPendingUploadsForAccount.size} uploads for account $accountName")
-            handlePendingUploads(currentAndPendingUploadsForAccount, accountName)
-            currentAndPendingUploadsForAccount =
-                uploadsStorageManager.getCurrentAndPendingUploadsForAccount(MAX_UPLOADS_QUERY, accountName)
+        /*
+         * As pages are retrieved by sorting uploads by ID, if new uploads are added while uploading the current ones,
+         * they will be present in the pages that follow.
+         */
+        var currentPage = uploadsStorageManager.getCurrentAndPendingUploadsForAccountPageAscById(-1, accountName)
+        while (currentPage.isNotEmpty()) {
+            Log_OC.d(TAG, "Handling ${currentPage.size} uploads for account $accountName")
+            val lastId = currentPage.last().uploadId
+            handlePendingUploads(currentPage, accountName)
+            currentPage =
+                uploadsStorageManager.getCurrentAndPendingUploadsForAccountPageAscById(lastId, accountName)
         }
 
         Log_OC.d(TAG, "No more pending uploads for account $accountName, stopping work")
         return Result.success()
     }
 
-    private fun handlePendingUploads(uploads: Array<OCUpload>, accountName: String) {
+    private fun handlePendingUploads(uploads: List<OCUpload>, accountName: String) {
         val user = userAccountManager.getUser(accountName)
 
         for (upload in uploads) {
@@ -240,7 +243,6 @@ class FilesUploadWorker(
 
     companion object {
         val TAG: String = FilesUploadWorker::class.java.simpleName
-        private const val MAX_UPLOADS_QUERY = 100
         private const val FOREGROUND_SERVICE_ID: Int = 412
         private const val MAX_PROGRESS: Int = 100
         const val ACCOUNT = "data_account"

+ 48 - 14
app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java

@@ -409,17 +409,32 @@ public class UploadsStorageManager extends Observable {
         return uploads.toArray(new OCUpload[0]);
     }
 
-
     @NonNull
     private List<OCUpload> getUploadPage(final long afterId, @Nullable String selection, @Nullable String... selectionArgs) {
+        return getUploadPage(afterId, true, selection, selectionArgs);
+    }
+
+    @NonNull
+    private List<OCUpload> getUploadPage(final long afterId, final boolean descending, @Nullable String selection, @Nullable String... selectionArgs) {
         List<OCUpload> uploads = new ArrayList<>();
         String pageSelection = selection;
         String[] pageSelectionArgs = selectionArgs;
+
+        String idComparator;
+        String sortDirection;
+        if (descending) {
+            sortDirection = "DESC";
+            idComparator = "<";
+        } else {
+            sortDirection = "ASC";
+            idComparator = ">";
+        }
+
         if (afterId >= 0) {
             if (selection != null) {
-                pageSelection = "(" + selection + ") AND _id < ?";
+                pageSelection = "(" + selection + ") AND _id " + idComparator + " ?";
             } else {
-                pageSelection = "_id < ?";
+                pageSelection = "_id " + idComparator + " ?";
             }
             if (selectionArgs != null) {
                 pageSelectionArgs = Arrays.copyOf(selectionArgs, selectionArgs.length + 1);
@@ -436,7 +451,7 @@ public class UploadsStorageManager extends Observable {
             null,
             pageSelection,
             pageSelectionArgs,
-            String.format(Locale.ENGLISH, "_id DESC LIMIT %d", QUERY_PAGE_SIZE)
+            String.format(Locale.ENGLISH, "_id " + sortDirection + " LIMIT %d", QUERY_PAGE_SIZE)
                                 );
 
         if (c != null) {
@@ -509,21 +524,40 @@ public class UploadsStorageManager extends Observable {
                           accountName);
     }
 
+    /**
+     * Gets a page of uploads after <code>afterId</code>, where uploads are sorted by ascending upload id.
+     * <p>
+     * If <code>afterId</code> is -1, returns the first page
+     */
+    public List<OCUpload> getCurrentAndPendingUploadsForAccountPageAscById(final long afterId, final @NonNull String accountName) {
+        final String selection = ProviderTableMeta.UPLOADS_STATUS + "==" + UploadStatus.UPLOAD_IN_PROGRESS.value +
+            " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+            "==" + UploadResult.DELAYED_FOR_WIFI.getValue() +
+            " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+            "==" + UploadResult.LOCK_FAILED.getValue() +
+            " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+            "==" + UploadResult.DELAYED_FOR_CHARGING.getValue() +
+            " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+            "==" + UploadResult.DELAYED_IN_POWER_SAVE_MODE.getValue() +
+            " AND " + ProviderTableMeta.UPLOADS_ACCOUNT_NAME + "== ?";
+        return getUploadPage(afterId, false, selection, accountName);
+    }
+
     /**
      * Get all failed uploads.
      */
     public OCUpload[] getFailedUploads() {
         return getUploads("(" + ProviderTableMeta.UPLOADS_STATUS + "== ?" +
-                " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                        "==" + UploadResult.DELAYED_FOR_WIFI.getValue() +
-                        " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                        "==" + UploadResult.LOCK_FAILED.getValue() +
-                        " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                        "==" + UploadResult.DELAYED_FOR_CHARGING.getValue() +
-                        " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                        "==" + UploadResult.DELAYED_IN_POWER_SAVE_MODE.getValue() +
-                        " ) AND " + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                        "!= " + UploadResult.VIRUS_DETECTED.getValue()
+                              " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+                              "==" + UploadResult.DELAYED_FOR_WIFI.getValue() +
+                              " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+                              "==" + UploadResult.LOCK_FAILED.getValue() +
+                              " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+                              "==" + UploadResult.DELAYED_FOR_CHARGING.getValue() +
+                              " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+                              "==" + UploadResult.DELAYED_IN_POWER_SAVE_MODE.getValue() +
+                              " ) AND " + ProviderTableMeta.UPLOADS_LAST_RESULT +
+                              "!= " + UploadResult.VIRUS_DETECTED.getValue()
             , String.valueOf(UploadStatus.UPLOAD_FAILED.value));
     }