diff --git a/src/main/java/com/owncloud/android/files/services/FileUploader.java b/src/main/java/com/owncloud/android/files/services/FileUploader.java index 9b987d34a46f..4ba4c08567df 100644 --- a/src/main/java/com/owncloud/android/files/services/FileUploader.java +++ b/src/main/java/com/owncloud/android/files/services/FileUploader.java @@ -80,6 +80,7 @@ import java.io.File; import java.util.AbstractList; +import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.Map; @@ -88,6 +89,7 @@ import javax.annotation.Nullable; import javax.inject.Inject; +import androidx.annotation.GuardedBy; import androidx.annotation.NonNull; import androidx.core.app.NotificationCompat; import dagger.android.AndroidInjection; @@ -176,11 +178,14 @@ public class FileUploader extends Service public static final int LOCAL_BEHAVIOUR_FORGET = 2; public static final int LOCAL_BEHAVIOUR_DELETE = 3; + private final Object uploadStartLock = new Object(); + private Looper mServiceLooper; private ServiceHandler mServiceHandler; private IBinder mBinder; - private OwnCloudClient mUploadClient; + @GuardedBy("uploadStartLock") private Account mCurrentAccount; + @GuardedBy("uploadStartLock") private FileDataStorageManager mStorageManager; //since there can be only one instance of an Android service, there also just one db connection. @Inject UploadsStorageManager mUploadsStorageManager; @@ -192,6 +197,7 @@ public class FileUploader extends Service /** * {@link UploadFileOperation} object of ongoing upload. Can be null. Note: There can only be one concurrent upload! */ + @GuardedBy("uploadStartLock") private UploadFileOperation mCurrentUpload; private NotificationManager mNotificationManager; @@ -212,8 +218,10 @@ public static String getUploadFinishMessage() { @Override public void onRenameUpload() { - mUploadsStorageManager.updateDatabaseUploadStart(mCurrentUpload); - sendBroadcastUploadStarted(mCurrentUpload); + synchronized (uploadStartLock) { + mUploadsStorageManager.updateDatabaseUploadStart(mCurrentUpload); + sendBroadcastUploadStarted(mCurrentUpload); + } } /** @@ -768,11 +776,13 @@ public boolean onUnbind(Intent intent) { @Override public void onAccountsUpdated(Account[] accounts) { - // Review current upload, and cancel it if its account doen't exist - if (mCurrentUpload != null && !accountManager.exists(mCurrentUpload.getAccount())) { - mCurrentUpload.cancel(); + synchronized (uploadStartLock) { + // Review current upload, and cancel it if its account doen't exist + if (mCurrentUpload != null && !accountManager.exists(mCurrentUpload.getAccount())) { + mCurrentUpload.cancel(); + } + // The rest of uploads are cancelled when they try to start } - // The rest of uploads are cancelled when they try to start } /** @@ -786,7 +796,8 @@ public class FileUploaderBinder extends Binder implements OnDatatransferProgress * Map of listeners that will be reported about progress of uploads from a * {@link FileUploaderBinder} instance */ - private Map mBoundListeners = new HashMap<>(); + private final Map mBoundListeners = + Collections.synchronizedMap(new HashMap<>()); /** * Cancels a pending or current upload of a remote file. @@ -817,15 +828,18 @@ public void cancel(OCUpload storedUpload) { * Setting result code will pause rather than cancel the job */ private void cancel(String accountName, String remotePath, @Nullable ResultCode resultCode ) { - Pair removeResult = + UploadFileOperation upload; + synchronized (uploadStartLock) { + Pair removeResult = mPendingUploads.remove(accountName, remotePath); - UploadFileOperation upload = removeResult.first; - if (upload == null && + upload = removeResult.first; + if (upload == null && mCurrentUpload != null && mCurrentAccount != null && mCurrentUpload.getRemotePath().startsWith(remotePath) && accountName.equals(mCurrentAccount.name)) { - upload = mCurrentUpload; + upload = mCurrentUpload; + } } if (upload != null) { @@ -849,10 +863,12 @@ private void cancel(String accountName, String remotePath, @Nullable ResultCode public void cancel(Account account) { Log_OC.d(TAG, "Account= " + account.name); - if (mCurrentUpload != null) { - Log_OC.d(TAG, "Current Upload Account= " + mCurrentUpload.getAccount().name); - if (mCurrentUpload.getAccount().name.equals(account.name)) { - mCurrentUpload.cancel(); + synchronized (uploadStartLock) { + if (mCurrentUpload != null) { + Log_OC.d(TAG, "Current Upload Account= " + mCurrentUpload.getAccount().name); + if (mCurrentUpload.getAccount().name.equals(account.name)) { + mCurrentUpload.cancel(); + } } } // Cancel pending uploads @@ -885,13 +901,15 @@ public boolean isUploading(Account account, OCFile file) { } public boolean isUploadingNow(OCUpload upload) { - return + synchronized (uploadStartLock) { + return upload != null && - mCurrentAccount != null && - mCurrentUpload != null && - upload.getAccountName().equals(mCurrentAccount.name) && - upload.getRemotePath().equals(mCurrentUpload.getRemotePath()) - ; + mCurrentAccount != null && + mCurrentUpload != null && + upload.getAccountName().equals(mCurrentAccount.name) && + upload.getRemotePath().equals(mCurrentUpload.getRemotePath()) + ; + } } /** @@ -946,8 +964,10 @@ public void removeDatatransferProgressListener( return; } String targetKey = buildRemoteName(account.name, file.getRemotePath()); - if (mBoundListeners.get(targetKey) == listener) { - mBoundListeners.remove(targetKey); + synchronized (mBoundListeners) { + if (mBoundListeners.get(targetKey) == listener) { + mBoundListeners.remove(targetKey); + } } } @@ -965,8 +985,10 @@ public void removeDatatransferProgressListener( return; } String targetKey = buildRemoteName(ocUpload.getAccountName(), ocUpload.getRemotePath()); - if (mBoundListeners.get(targetKey) == listener) { - mBoundListeners.remove(targetKey); + synchronized (mBoundListeners) { + if (mBoundListeners.get(targetKey) == listener) { + mBoundListeners.remove(targetKey); + } } } @@ -1012,6 +1034,16 @@ private String buildRemoteName(String accountName, String remotePath) { return accountName + remotePath; } + /** + * Returns the object whose intrinsic lock is used to synchronize starting of the next upload. + * While the lock is held no upload can be started (starting an upload requires acquiring this lock), + * it can be used to ensure having a consistent view of the current upload across multiple calls. + * + * @return lock object to use in synchronize + */ + public Object uploadStartLock() { + return uploadStartLock; + } } @@ -1059,44 +1091,54 @@ public void handleMessage(Message msg) { */ public void uploadFile(String uploadKey) { - mCurrentUpload = mPendingUploads.get(uploadKey); - - if (mCurrentUpload != null) { + UploadFileOperation currentUpload; + Account currentAccount; - /// Check account existence - if (!accountManager.exists(mCurrentUpload.getAccount())) { - Log_OC.w(TAG, "Account " + mCurrentUpload.getAccount().name + - " does not exist anymore -> cancelling all its uploads"); - cancelUploadsForAccount(mCurrentUpload.getAccount()); + synchronized (uploadStartLock) { + currentUpload = mPendingUploads.get(uploadKey); + if (currentUpload == null) { return; } + mCurrentUpload = currentUpload; + + /// prepare client object to send the request to the ownCloud server + if (mCurrentAccount == null || !mCurrentAccount.equals(currentUpload.getAccount())) { + mCurrentAccount = currentUpload.getAccount(); + mStorageManager = new FileDataStorageManager( + mCurrentAccount, + getContentResolver() + ); + } // else, reuse storage manager from previous operation + currentAccount = mCurrentAccount; + } - /// OK, let's upload - mUploadsStorageManager.updateDatabaseUploadStart(mCurrentUpload); - notifyUploadStart(mCurrentUpload); + /// Check account existence + if (!accountManager.exists(currentUpload.getAccount())) { + Log_OC.w(TAG, "Account " + currentUpload.getAccount().name + + " does not exist anymore -> cancelling all its uploads"); + cancelUploadsForAccount(currentUpload.getAccount()); + return; + } - sendBroadcastUploadStarted(mCurrentUpload); + /// OK, let's upload + mUploadsStorageManager.updateDatabaseUploadStart(currentUpload); - RemoteOperationResult uploadResult = null; + notifyUploadStart(currentUpload); - try { - /// prepare client object to send the request to the ownCloud server - if (mCurrentAccount == null || !mCurrentAccount.equals(mCurrentUpload.getAccount())) { - mCurrentAccount = mCurrentUpload.getAccount(); - mStorageManager = new FileDataStorageManager( - mCurrentAccount, - getContentResolver() - ); - } // else, reuse storage manager from previous operation + sendBroadcastUploadStarted(currentUpload); - // always get client from client manager, to get fresh credentials in case of update - OwnCloudAccount ocAccount = new OwnCloudAccount( - mCurrentAccount, - this - ); - mUploadClient = OwnCloudClientManagerFactory.getDefaultSingleton(). - getClientFor(ocAccount, this); + RemoteOperationResult uploadResult = null; + + try { + + // always get client from client manager, to get fresh credentials in case of update + OwnCloudAccount ocAccount = new OwnCloudAccount( + currentAccount, + this + ); + OwnCloudClient uploadClient = OwnCloudClientManagerFactory.getDefaultSingleton(). + getClientFor(ocAccount, this); // // If parent folder is encrypted, upload file encrypted @@ -1106,48 +1148,47 @@ public void uploadFile(String uploadKey) { // UploadEncryptedFileOperation uploadEncryptedFileOperation = // new UploadEncryptedFileOperation(parent, mCurrentUpload); // -// uploadResult = uploadEncryptedFileOperation.execute(mUploadClient, mStorageManager); +// uploadResult = uploadEncryptedFileOperation.execute(uploadClient, mStorageManager); // } else { - /// perform the regular upload - uploadResult = mCurrentUpload.execute(mUploadClient, mStorageManager); + /// perform the regular upload + uploadResult = currentUpload.execute(uploadClient, mStorageManager); // } - } catch (Exception e) { - Log_OC.e(TAG, "Error uploading", e); - uploadResult = new RemoteOperationResult(e); - - } finally { - Pair removeResult; - if (mCurrentUpload.wasRenamed()) { - removeResult = mPendingUploads.removePayload( - mCurrentAccount.name, - mCurrentUpload.getOldFile().getRemotePath() - ); - // TODO: grant that name is also updated for mCurrentUpload.getOCUploadId - - } else { - removeResult = mPendingUploads.removePayload(mCurrentAccount.name, - mCurrentUpload.getDecryptedRemotePath()); - } - - mUploadsStorageManager.updateDatabaseUploadResult(uploadResult, mCurrentUpload); + } catch (Exception e) { + Log_OC.e(TAG, "Error uploading", e); + uploadResult = new RemoteOperationResult(e); - /// notify result - notifyUploadResult(mCurrentUpload, uploadResult); + } finally { + Pair removeResult; + if (currentUpload.wasRenamed()) { + removeResult = mPendingUploads.removePayload( + currentAccount.name, + currentUpload.getOldFile().getRemotePath() + ); + // TODO: grant that name is also updated for mCurrentUpload.getOCUploadId - sendBroadcastUploadFinished(mCurrentUpload, uploadResult, removeResult.second); + } else { + removeResult = mPendingUploads.removePayload(currentAccount.name, + currentUpload.getDecryptedRemotePath()); } - // generate new Thumbnail - final ThumbnailsCacheManager.ThumbnailGenerationTask task = - new ThumbnailsCacheManager.ThumbnailGenerationTask(mStorageManager, mCurrentAccount); + mUploadsStorageManager.updateDatabaseUploadResult(uploadResult, currentUpload); - File file = new File(mCurrentUpload.getOriginalStoragePath()); - String remoteId = mCurrentUpload.getFile().getRemoteId(); + /// notify result + notifyUploadResult(currentUpload, uploadResult); - task.execute(new ThumbnailsCacheManager.ThumbnailGenerationTaskObject(file, remoteId)); + sendBroadcastUploadFinished(currentUpload, uploadResult, removeResult.second); } + + // generate new Thumbnail + final ThumbnailsCacheManager.ThumbnailGenerationTask task = + new ThumbnailsCacheManager.ThumbnailGenerationTask(mStorageManager, currentAccount); + + File file = new File(currentUpload.getOriginalStoragePath()); + String remoteId = currentUpload.getFile().getRemoteId(); + + task.execute(new ThumbnailsCacheManager.ThumbnailGenerationTaskObject(file, remoteId)); } diff --git a/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java b/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java index 409437867a9d..5bccff0e6212 100755 --- a/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java +++ b/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java @@ -173,7 +173,7 @@ public UploadListAdapter(final FileActivity fileActivity, parentActivity.getString(R.string.uploads_view_group_current_uploads)) { @Override public void refresh() { - fixAndSortItems(uploadsStorageManager.getCurrentAndPendingUploadsForCurrentAccount()); + setItems(uploadsStorageManager.getCurrentAndPendingUploadsForCurrentAccount()); } }; @@ -181,7 +181,7 @@ public void refresh() { parentActivity.getString(R.string.uploads_view_group_failed_uploads)) { @Override public void refresh() { - fixAndSortItems(uploadsStorageManager.getFailedButNotDelayedUploadsForCurrentAccount()); + setItems(uploadsStorageManager.getFailedButNotDelayedUploadsForCurrentAccount()); } }; @@ -189,7 +189,7 @@ public void refresh() { parentActivity.getString(R.string.uploads_view_group_finished_uploads)) { @Override public void refresh() { - fixAndSortItems(uploadsStorageManager.getFinishedUploadsForCurrentAccount()); + setItems(uploadsStorageManager.getFinishedUploadsForCurrentAccount()); } }; loadUploadItemsFromDb(); @@ -587,10 +587,26 @@ public SectionedViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int vie public void loadUploadItemsFromDb() { Log_OC.d(TAG, "loadUploadItemsFromDb"); + /* TODO: this fires multiple DB queries and the FileUploader can concurrently change the DB + * the uploadStartLock cannot help here because + * 1. it does not prevent the running upload from finish/failing + * 2. it synchronizes memory access to the FileUploader instance, not DB state + * 3. it is dangerous to hold locks while performing long-running operations such as DB access + * The uploads lists refresh should be refactored to (transactionally) read from a single source for OCUploads, + * it should not concurrently access DB and memory state, which cannot be easily kept consistent. + */ for (UploadGroup group : uploadGroups) { group.refresh(); } + // we synchronize with the binder lock to avoid it changing the current upload and making the lists inconsistent + FileUploader.FileUploaderBinder binder = parentActivity.getFileUploaderBinder(); + synchronized (binder.uploadStartLock()) { + for (UploadGroup group : uploadGroups) { + group.fixAndSortItems(binder); + } + } + notifyDataSetChanged(); } @@ -708,15 +724,11 @@ public void setItems(OCUpload... items) { this.items = items; } - void fixAndSortItems(OCUpload... array) { - FileUploader.FileUploaderBinder binder = parentActivity.getFileUploaderBinder(); - - for (OCUpload upload : array) { - upload.setDataFixed(binder); + void fixAndSortItems(FileUploader.FileUploaderBinder binder) { + for (OCUpload upload : items) { + upload.setDataFixed(binder); } - Arrays.sort(array, comparator); - - setItems(array); + Arrays.sort(items, comparator); } private int getGroupItemCount() {