From a3e21d719b45c9c710caeba8aa40ee04c86eca3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Brey?= Date: Thu, 1 Dec 2022 16:17:22 +0100 Subject: [PATCH 1/3] UploadsStorageManager: extract getUploadsPage from getUploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First step for making it accessible from outside Signed-off-by: Álvaro Brey --- .../datamodel/UploadsStorageManager.java | 122 ++++++++++-------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java b/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java index e824254d12..d1208d15c0 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java @@ -56,8 +56,7 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; /** - * Database helper for storing list of files to be uploaded, including status - * information for each file. + * Database helper for storing list of files to be uploaded, including status information for each file. */ public class UploadsStorageManager extends Observable { private static final String TAG = UploadsStorageManager.class.getSimpleName(); @@ -65,13 +64,15 @@ public class UploadsStorageManager extends Observable { private static final String AND = " AND "; private static final int SINGLE_RESULT = 1; + private static final long QUERY_PAGE_SIZE = 100; + private final ContentResolver contentResolver; private final CurrentAccountProvider currentAccountProvider; public UploadsStorageManager( CurrentAccountProvider currentAccountProvider, ContentResolver contentResolver - ) { + ) { if (contentResolver == null) { throw new IllegalArgumentException("Cannot create an instance with a NULL contentResolver"); } @@ -367,69 +368,30 @@ public class UploadsStorageManager extends Observable { return getUploads(0, selection, selectionArgs); } + private OCUpload[] getUploads(final int limit, @Nullable String selection, @Nullable String... selectionArgs) { ArrayList uploads = new ArrayList<>(); - final long pageSize = 100; long page = 0; long rowsRead; long rowsTotal = 0; long lastRowID = -1; do { - String pageSelection = selection; - String[] pageSelectionArgs = selectionArgs; - if (page > 0 && lastRowID >= 0) { - if (selection != null) { - pageSelection = "(" + selection + ") AND _id < ?"; - } else { - pageSelection = "_id < ?"; - } - if (selectionArgs != null) { - pageSelectionArgs = Arrays.copyOf(selectionArgs, selectionArgs.length + 1); - } else { - pageSelectionArgs = new String[1]; - } - pageSelectionArgs[pageSelectionArgs.length - 1] = String.valueOf(lastRowID); - Log_OC.d(TAG, String.format(Locale.ENGLISH, "QUERY: %s ROWID: %d", pageSelection, lastRowID)); - } else { - Log_OC.d(TAG, String.format(Locale.ENGLISH, "QUERY: %s ROWID: %d", selection, lastRowID)); - } - rowsRead = 0; - - Cursor c = getDB().query( - ProviderTableMeta.CONTENT_URI_UPLOADS, - null, - pageSelection, - pageSelectionArgs, - String.format(Locale.ENGLISH, "_id DESC LIMIT %d", pageSize) - ); - - if (c != null) { - if (c.moveToFirst()) { - do { - rowsRead++; - rowsTotal++; - lastRowID = c.getLong(c.getColumnIndexOrThrow(ProviderTableMeta._ID)); - OCUpload upload = createOCUploadFromCursor(c); - if (upload == null) { - Log_OC.e(TAG, "OCUpload could not be created from cursor"); - } else { - uploads.add(upload); - } - } while (c.moveToNext() && !c.isAfterLast()); - } - c.close(); - Log_OC.v(TAG, String.format(Locale.ENGLISH, - "getUploads() got %d rows from page %d, %d rows total so far, last ID %d", - rowsRead, - page, - rowsTotal, - lastRowID - )); - page += 1; - } else { - break; + final List uploadsPage = getUploadPage(lastRowID, selection, selectionArgs); + rowsRead = uploadsPage.size(); + rowsTotal += rowsRead; + if (!uploadsPage.isEmpty()) { + lastRowID = uploadsPage.get(uploadsPage.size() - 1).getUploadId(); } + Log_OC.v(TAG, String.format(Locale.ENGLISH, + "getUploads() got %d rows from page %d, %d rows total so far, last ID %d", + rowsRead, + page, + rowsTotal, + lastRowID + )); + uploads.addAll(uploadsPage); + page++; } while (rowsRead > 0 && (limit <= 0 || rowsRead < limit)); if (limit > 0 && uploads.size() > limit) { @@ -447,6 +409,52 @@ public class UploadsStorageManager extends Observable { return uploads.toArray(new OCUpload[0]); } + + @NonNull + private List getUploadPage(final long afterId, @Nullable String selection, @Nullable String... selectionArgs) { + List uploads = new ArrayList<>(); + String pageSelection = selection; + String[] pageSelectionArgs = selectionArgs; + if (afterId >= 0) { + if (selection != null) { + pageSelection = "(" + selection + ") AND _id < ?"; + } else { + pageSelection = "_id < ?"; + } + if (selectionArgs != null) { + pageSelectionArgs = Arrays.copyOf(selectionArgs, selectionArgs.length + 1); + } else { + pageSelectionArgs = new String[1]; + } + pageSelectionArgs[pageSelectionArgs.length - 1] = String.valueOf(afterId); + Log_OC.d(TAG, String.format(Locale.ENGLISH, "QUERY: %s ROWID: %d", pageSelection, afterId)); + } else { + Log_OC.d(TAG, String.format(Locale.ENGLISH, "QUERY: %s ROWID: %d", selection, afterId)); + } + Cursor c = getDB().query( + ProviderTableMeta.CONTENT_URI_UPLOADS, + null, + pageSelection, + pageSelectionArgs, + String.format(Locale.ENGLISH, "_id DESC LIMIT %d", QUERY_PAGE_SIZE) + ); + + if (c != null) { + if (c.moveToFirst()) { + do { + OCUpload upload = createOCUploadFromCursor(c); + if (upload == null) { + Log_OC.e(TAG, "OCUpload could not be created from cursor"); + } else { + uploads.add(upload); + } + } while (c.moveToNext() && !c.isAfterLast()); + } + c.close(); + } + return uploads; + } + private OCUpload createOCUploadFromCursor(Cursor c) { OCUpload upload = null; if (c != null) { From f425045a316cca23f1b4fb0a8afd5f6b3cec98f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Brey?= Date: Thu, 1 Dec 2022 16:52:43 +0100 Subject: [PATCH 2/3] FilesUploadWorker: fix infinite loop when uploads failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../client/jobs/FilesUploadWorker.kt | 22 ++++--- .../datamodel/UploadsStorageManager.java | 62 ++++++++++++++----- 2 files changed, 60 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/com/nextcloud/client/jobs/FilesUploadWorker.kt b/app/src/main/java/com/nextcloud/client/jobs/FilesUploadWorker.kt index 02260139f2..e0c052071c 100644 --- a/app/src/main/java/com/nextcloud/client/jobs/FilesUploadWorker.kt +++ b/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, accountName: String) { + private fun handlePendingUploads(uploads: List, 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" diff --git a/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java b/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java index d1208d15c0..c637cbc0e0 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java +++ b/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 getUploadPage(final long afterId, @Nullable String selection, @Nullable String... selectionArgs) { + return getUploadPage(afterId, true, selection, selectionArgs); + } + + @NonNull + private List getUploadPage(final long afterId, final boolean descending, @Nullable String selection, @Nullable String... selectionArgs) { List 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 afterId, where uploads are sorted by ascending upload id. + *

+ * If afterId is -1, returns the first page + */ + public List 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)); } From 84044905d1ea6323f0af0b632f5e5adb06e62288 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Brey?= Date: Thu, 1 Dec 2022 17:00:02 +0100 Subject: [PATCH 3/3] UploadsStorageManager: remove in-memory limit for queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only added for FilesUploadWorker workaround that is no longer used Signed-off-by: Álvaro Brey --- .../datamodel/UploadsStorageManager.java | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java b/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java index c637cbc0e0..0aac8b984a 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java @@ -365,12 +365,7 @@ public class UploadsStorageManager extends Observable { } private OCUpload[] getUploads(@Nullable String selection, @Nullable String... selectionArgs) { - return getUploads(0, selection, selectionArgs); - } - - - private OCUpload[] getUploads(final int limit, @Nullable String selection, @Nullable String... selectionArgs) { - ArrayList uploads = new ArrayList<>(); + final List uploads = new ArrayList<>(); long page = 0; long rowsRead; long rowsTotal = 0; @@ -392,11 +387,8 @@ public class UploadsStorageManager extends Observable { )); uploads.addAll(uploadsPage); page++; - } while (rowsRead > 0 && (limit <= 0 || rowsRead < limit)); + } while (rowsRead > 0); - if (limit > 0 && uploads.size() > limit) { - uploads = new ArrayList<>(uploads.subList(0, limit)); - } Log_OC.v(TAG, String.format(Locale.ENGLISH, "getUploads() returning %d (%d) rows after reading %d pages", @@ -507,11 +499,7 @@ public class UploadsStorageManager extends Observable { } public OCUpload[] getCurrentAndPendingUploadsForAccount(final @NonNull String accountName) { - return getCurrentAndPendingUploadsForAccount(0, accountName); - } - - public OCUpload[] getCurrentAndPendingUploadsForAccount(final int limit, final @NonNull String accountName) { - return getUploads(limit, ProviderTableMeta.UPLOADS_STATUS + "==" + UploadStatus.UPLOAD_IN_PROGRESS.value + + return getUploads(ProviderTableMeta.UPLOADS_STATUS + "==" + UploadStatus.UPLOAD_IN_PROGRESS.value + " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT + "==" + UploadResult.DELAYED_FOR_WIFI.getValue() + " OR " + ProviderTableMeta.UPLOADS_LAST_RESULT +