From d915e4ca0feeb0e8fdefde60a6ecd8d449023cab Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Tue, 10 May 2022 18:48:06 +0200 Subject: [PATCH 1/5] SynchronizationService can only have a single SyncRequest per file in queue - Update SyncRequest.equals() - Update SynchronizationService: * Add a synchronized block on queueOperation(...) & queueOperations(...) * Start by removing SyncRequest in queue before to add it again. Thanks to SyncRequest.equals() we'll have a single SyncRequest in queue by file. --- .../foundation/e/drive/models/SyncRequest.java | 7 +++++-- .../drive/services/SynchronizationService.java | 18 ++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncRequest.java b/app/src/main/java/foundation/e/drive/models/SyncRequest.java index e2412cdf..758060de 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncRequest.java +++ b/app/src/main/java/foundation/e/drive/models/SyncRequest.java @@ -32,8 +32,11 @@ public class SyncRequest { @Override public boolean equals(@Nullable Object obj) { if (obj instanceof SyncRequest) { - return (syncedFileState.getId() == ((SyncRequest) obj).syncedFileState.getId() ); + final SyncedFileState syncedFileState = ((SyncRequest) obj).getSyncedFileState(); + + return (syncedFileState.getLocalPath().equals(syncedFileState.getLocalPath()) + || syncedFileState.getRemotePath().equals(syncedFileState.getRemotePath())); } - return super.equals(obj); + return false; } } diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index 72cee50c..d3b63c61 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -102,11 +102,19 @@ public class SynchronizationService extends Service implements OnRemoteOperation } public boolean queueOperation(SyncRequest request){ - return syncedRequestQueue.add(request); + synchronized (syncedRequestQueue) { + syncedRequestQueue.remove(request); + syncedRequestQueue.add(request); + } + return true; } public boolean queueOperations(Collection requests){ - return syncedRequestQueue.addAll(requests); + synchronized (syncedRequestQueue) { + syncedRequestQueue.removeAll(requests); + syncedRequestQueue.addAll(requests); + } + return true; } public void startSynchronization(){ @@ -121,10 +129,12 @@ public class SynchronizationService extends Service implements OnRemoteOperation final boolean meteredNetworkAllowed = CommonUtils.isMeteredNetworkAllowed(account); if (!threadWorkingState[threadIndex] && CommonUtils.haveNetworkConnection(getApplicationContext(), meteredNetworkAllowed)) { //check if the thread corresponding to threadIndex isn't already working - final SyncRequest request = this.syncedRequestQueue.poll(); //return null if deque is empty + final SyncRequest request; + synchronized (syncedRequestQueue) { + request = syncedRequestQueue.poll(); //return null if deque is empty + } final RemoteOperation operation = this.createRemoteOperation(request); - if (operation != null) { Log.v(TAG, " an operation has been poll from queue"); -- GitLab From 8101ff9dd6613faec3bfa85d956ca852aab844e6 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 11 May 2022 12:17:28 +0200 Subject: [PATCH 2/5] Extract some thread's data from SynchronizationService into SyncThread class - add a new SyncThread class to encapsulate SyncRequest, remoteOperation, threadIndex, and thread is running. - Update SynchronizationService to use SyncThread. - Remove old fields: startedOperations, threadWorkingState - move method 'createRemoteOperation(SyncRequest request)' into SyncThread - remove unused import - add 'synchronized' block & lock object in SynchronizationService to access queue & running thread - SyncThread has been renamed SyncThreadHolder and doesn't implement Thread anymore. - remove useless call to CommonUtils.createNotificationChannel() --- .../e/drive/models/SyncThreadHolder.java | 72 ++++++++++++ .../services/SynchronizationService.java | 111 ++++++++---------- 2 files changed, 122 insertions(+), 61 deletions(-) create mode 100644 app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java diff --git a/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java b/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java new file mode 100644 index 00000000..32886ada --- /dev/null +++ b/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java @@ -0,0 +1,72 @@ +package foundation.e.drive.models; + +import android.content.Context; +import android.os.Handler; + +import com.owncloud.android.lib.common.OwnCloudClient; +import com.owncloud.android.lib.common.operations.OnRemoteOperationListener; +import com.owncloud.android.lib.common.operations.RemoteOperation; + +import foundation.e.drive.operations.DownloadFileOperation; +import foundation.e.drive.operations.UploadFileOperation; + +public class SyncThreadHolder { + private final RemoteOperation remoteOperation; + private final SyncRequest syncRequest; + private boolean isRunning; + private final int threadIndex; + + public SyncThreadHolder(int threadIndex, SyncRequest request, Context context){ + super(); + this.threadIndex = threadIndex; + this.syncRequest = request; + remoteOperation = createRemoteOperation(syncRequest, context); + isRunning = false; + } + + public RemoteOperation getRemoteOperation() { + return remoteOperation; + } + + public SyncRequest getSyncRequest() { + return syncRequest; + } + + public boolean isRunning() { + return isRunning; + } + + public int getThreadIndex() { + return threadIndex; + } + + public void setRunning(boolean running) { + isRunning = running; + } + + private RemoteOperation createRemoteOperation(SyncRequest request, Context context){ + final RemoteOperation operation; + switch (request.getOperationType()){ + case UPLOAD: + final SyncedFileState sfs = request.getSyncedFileState(); + operation = new UploadFileOperation(sfs, context); + break; + case DOWNLOAD: + final DownloadRequest downloadRequest = (DownloadRequest) request; + operation = new DownloadFileOperation(downloadRequest.getRemoteFile(), downloadRequest.getSyncedFileState(), context); + break; + case REMOTE_DELETE: + default: + operation = null; + break; + } + return operation; + } + + + public SyncThreadHolder executeOperation(OwnCloudClient client, OnRemoteOperationListener listener, Handler handler){ + isRunning = true; + remoteOperation.execute(client, listener, handler); + return this; + } +} diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index d3b63c61..3870d43f 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -9,9 +9,6 @@ package foundation.e.drive.services; import android.accounts.Account; import android.accounts.AccountManager; -import android.app.Notification; -import android.app.NotificationManager; -import android.app.PendingIntent; import android.app.Service; import android.content.Context; import android.content.Intent; @@ -24,7 +21,6 @@ import android.os.Message; import android.util.Log; import androidx.annotation.Nullable; -import androidx.core.app.NotificationCompat; import com.owncloud.android.lib.common.OwnCloudClient; import com.owncloud.android.lib.common.operations.OnRemoteOperationListener; @@ -34,14 +30,11 @@ import com.owncloud.android.lib.common.operations.RemoteOperationResult; import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Collection; -import java.util.Hashtable; import java.util.concurrent.ConcurrentLinkedDeque; -import foundation.e.drive.R; import foundation.e.drive.database.DbHelper; -import foundation.e.drive.models.DownloadRequest; import foundation.e.drive.models.SyncRequest; -import foundation.e.drive.models.SyncedFileState; +import foundation.e.drive.models.SyncThreadHolder; import foundation.e.drive.operations.DownloadFileOperation; import foundation.e.drive.operations.RemoveFileOperation; import foundation.e.drive.operations.UploadFileOperation; @@ -57,13 +50,13 @@ public class SynchronizationService extends Service implements OnRemoteOperation private final SynchronizationBinder binder = new SynchronizationBinder(); private ConcurrentLinkedDeque syncedRequestQueue; - private Hashtable startedOperations; //Operations which are running private Account account; private final int workerAmount = 4; - private boolean[] threadWorkingState; //State of the threads; true mean the thread is working - private Thread[] threadPool; + private SyncThreadHolder[] threadPool; private OwnCloudClient client; private OperationHandler handler; + private final Object queueLock = new Object(); + private final Object runningThreadLock = new Object(); @Override public int onStartCommand(Intent intent, int flags, int startId) { @@ -81,9 +74,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation } syncedRequestQueue = new ConcurrentLinkedDeque<>(); - startedOperations = new Hashtable<>(); - threadPool = new Thread[workerAmount]; - threadWorkingState = new boolean[workerAmount]; + threadPool = new SyncThreadHolder[workerAmount]; client = DavClientProvider.getInstance().getClientInstance(account, getApplicationContext()); handler = new OperationHandler(this); @@ -101,20 +92,32 @@ public class SynchronizationService extends Service implements OnRemoteOperation Log.w(TAG, "System is low on memory. Service might get killed."); } - public boolean queueOperation(SyncRequest request){ - synchronized (syncedRequestQueue) { + public void queueOperation(SyncRequest request){ + synchronized (runningThreadLock) { + for (SyncThreadHolder threadHolder : threadPool) { + if (threadHolder.getSyncRequest().equals(request)){ + return; + } + } + } + synchronized (queueLock) { syncedRequestQueue.remove(request); syncedRequestQueue.add(request); } - return true; } - public boolean queueOperations(Collection requests){ - synchronized (syncedRequestQueue) { + public void queueOperations(Collection requests){ + synchronized (runningThreadLock) { + final ArrayList runningRequests = new ArrayList<>(); + for (SyncThreadHolder threadHolder : threadPool) { + runningRequests.add(threadHolder.getSyncRequest()); + } + requests.removeAll(runningRequests); + } + synchronized (queueLock) { syncedRequestQueue.removeAll(requests); syncedRequestQueue.addAll(requests); } - return true; } public void startSynchronization(){ @@ -127,23 +130,22 @@ public class SynchronizationService extends Service implements OnRemoteOperation private void startWorker(int threadIndex){ if (syncedRequestQueue.isEmpty()) return; final boolean meteredNetworkAllowed = CommonUtils.isMeteredNetworkAllowed(account); - if (!threadWorkingState[threadIndex] && CommonUtils.haveNetworkConnection(getApplicationContext(), meteredNetworkAllowed)) { //check if the thread corresponding to threadIndex isn't already working - - final SyncRequest request; - synchronized (syncedRequestQueue) { - request = syncedRequestQueue.poll(); //return null if deque is empty + synchronized (runningThreadLock) { + if (threadPool[threadIndex].isRunning() || !CommonUtils.haveNetworkConnection(getApplicationContext(), meteredNetworkAllowed)) { //check if the thread corresponding to threadIndex isn't already working + return; } + } - final RemoteOperation operation = this.createRemoteOperation(request); - if (operation != null) { - Log.v(TAG, " an operation has been poll from queue"); + final SyncRequest request; + synchronized (queueLock) { + request = syncedRequestQueue.poll(); //return null if deque is empty + } - if (CommonUtils.isThisSyncAllowed(account, request.getSyncedFileState().isMediaType())) { - CommonUtils.createNotificationChannel(this); - startedOperations.put(operation, threadIndex); - threadPool[threadIndex] = operation.execute(client, this, handler); - threadWorkingState[threadIndex] = true; - } + Log.v(TAG, " an operation has been poll from queue"); + final SyncThreadHolder threadHolder = new SyncThreadHolder(threadIndex, request, getApplicationContext()); + if (CommonUtils.isThisSyncAllowed(account, request.getSyncedFileState().isMediaType())) { + synchronized (runningThreadLock) { + threadPool[threadIndex] = threadHolder.executeOperation(client, this, handler); } } } @@ -151,10 +153,16 @@ public class SynchronizationService extends Service implements OnRemoteOperation @Override public void onRemoteOperationFinish(RemoteOperation callerOperation, RemoteOperationResult result) { Log.d(TAG, "onRemoteOperationFinish()"); - Integer threadIndex = this.startedOperations.remove(callerOperation); - if (threadIndex != null) { - this.threadWorkingState[threadIndex] = false; - this.startWorker(threadIndex); + + synchronized (runningThreadLock) { + for (SyncThreadHolder thread : threadPool) { + if (callerOperation.equals(thread.getRemoteOperation())) { + final int threadIndex = thread.getThreadIndex(); + thread.setRunning(false); + startWorker(threadIndex); + break; + } + } } if (callerOperation instanceof RemoveFileOperation){ @@ -220,32 +228,11 @@ public class SynchronizationService extends Service implements OnRemoteOperation } } - private RemoteOperation createRemoteOperation(SyncRequest request){ - RemoteOperation operation; - switch (request.getOperationType()){ - case UPLOAD: - final SyncedFileState sfs = request.getSyncedFileState(); - operation = new UploadFileOperation(sfs, getApplicationContext()); - break; - case DOWNLOAD: - final DownloadRequest downloadRequest = (DownloadRequest) request; - operation = new DownloadFileOperation(downloadRequest.getRemoteFile(), downloadRequest.getSyncedFileState(), getApplicationContext()); - break; - case REMOTE_DELETE: - default: - operation = null; - break; - } - return operation; - } - - /** * Handler for the class */ static class OperationHandler extends Handler { private final String TAG = SynchronizationService.OperationHandler.class.getSimpleName(); - private final WeakReference serviceWeakRef; OperationHandler(SynchronizationService mOperationService){ @@ -257,8 +244,10 @@ public class SynchronizationService extends Service implements OnRemoteOperation Log.i(TAG, "handler.handleMessage()"); Bundle data = msg.getData(); - serviceWeakRef.get() - .threadWorkingState[data.getInt("thread index")] = data.getBoolean("mThreadWorkingState"); + synchronized (serviceWeakRef.get().runningThreadLock) { + serviceWeakRef.get() + .threadPool[data.getInt("thread index")].setRunning(data.getBoolean("mThreadWorkingState")); + } } } -- GitLab From bac888cf5e76448b74727d3ad5f7e0407e5bad73 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 11 May 2022 15:35:28 +0200 Subject: [PATCH 3/5] add Licence header to SyncThreadHolder --- .../foundation/e/drive/models/SyncThreadHolder.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java b/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java index 32886ada..ab43251b 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java +++ b/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java @@ -1,3 +1,10 @@ +/* + * Copyright © Vincent Bourgmayer (/e/ foundation). + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the GNU Public License v3.0 + * which accompanies this distribution, and is available at + * http://www.gnu.org/licenses/gpl.html + */ package foundation.e.drive.models; import android.content.Context; @@ -10,6 +17,9 @@ import com.owncloud.android.lib.common.operations.RemoteOperation; import foundation.e.drive.operations.DownloadFileOperation; import foundation.e.drive.operations.UploadFileOperation; +/** + * @author vincent Bourgmayer + */ public class SyncThreadHolder { private final RemoteOperation remoteOperation; private final SyncRequest syncRequest; -- GitLab From ef80de620f5f9050b2321aed9b66974c75cdaf08 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 11 May 2022 15:40:55 +0200 Subject: [PATCH 4/5] fix coding style --- .../java/foundation/e/drive/models/SyncThreadHolder.java | 6 +++--- .../foundation/e/drive/services/SynchronizationService.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java b/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java index ab43251b..7b412a34 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java +++ b/app/src/main/java/foundation/e/drive/models/SyncThreadHolder.java @@ -26,7 +26,7 @@ public class SyncThreadHolder { private boolean isRunning; private final int threadIndex; - public SyncThreadHolder(int threadIndex, SyncRequest request, Context context){ + public SyncThreadHolder(int threadIndex, SyncRequest request, Context context) { super(); this.threadIndex = threadIndex; this.syncRequest = request; @@ -54,7 +54,7 @@ public class SyncThreadHolder { isRunning = running; } - private RemoteOperation createRemoteOperation(SyncRequest request, Context context){ + private RemoteOperation createRemoteOperation(SyncRequest request, Context context) { final RemoteOperation operation; switch (request.getOperationType()){ case UPLOAD: @@ -74,7 +74,7 @@ public class SyncThreadHolder { } - public SyncThreadHolder executeOperation(OwnCloudClient client, OnRemoteOperationListener listener, Handler handler){ + public SyncThreadHolder executeOperation(OwnCloudClient client, OnRemoteOperationListener listener, Handler handler) { isRunning = true; remoteOperation.execute(client, listener, handler); return this; diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index 3870d43f..9833f021 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -95,7 +95,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation public void queueOperation(SyncRequest request){ synchronized (runningThreadLock) { for (SyncThreadHolder threadHolder : threadPool) { - if (threadHolder.getSyncRequest().equals(request)){ + if (threadHolder.getSyncRequest().equals(request)) { return; } } -- GitLab From f1a40837a903d18672b9876fd7669b13523bf681 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 11 May 2022 16:27:19 +0200 Subject: [PATCH 5/5] fix NPE in SynchronizationService --- .../services/SynchronizationService.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index 9833f021..15231352 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -95,7 +95,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation public void queueOperation(SyncRequest request){ synchronized (runningThreadLock) { for (SyncThreadHolder threadHolder : threadPool) { - if (threadHolder.getSyncRequest().equals(request)) { + if (threadHolder != null && threadHolder.getSyncRequest().equals(request)) { return; } } @@ -103,6 +103,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation synchronized (queueLock) { syncedRequestQueue.remove(request); syncedRequestQueue.add(request); + queueLock.notify(); } } @@ -110,7 +111,9 @@ public class SynchronizationService extends Service implements OnRemoteOperation synchronized (runningThreadLock) { final ArrayList runningRequests = new ArrayList<>(); for (SyncThreadHolder threadHolder : threadPool) { - runningRequests.add(threadHolder.getSyncRequest()); + if (threadHolder != null) { + runningRequests.add(threadHolder.getSyncRequest()); + } } requests.removeAll(runningRequests); } @@ -131,7 +134,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation if (syncedRequestQueue.isEmpty()) return; final boolean meteredNetworkAllowed = CommonUtils.isMeteredNetworkAllowed(account); synchronized (runningThreadLock) { - if (threadPool[threadIndex].isRunning() || !CommonUtils.haveNetworkConnection(getApplicationContext(), meteredNetworkAllowed)) { //check if the thread corresponding to threadIndex isn't already working + if ((threadPool[threadIndex] != null && threadPool[threadIndex].isRunning() ) || !CommonUtils.haveNetworkConnection(getApplicationContext(), meteredNetworkAllowed)) { //check if the thread corresponding to threadIndex isn't already working return; } } @@ -156,7 +159,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation synchronized (runningThreadLock) { for (SyncThreadHolder thread : threadPool) { - if (callerOperation.equals(thread.getRemoteOperation())) { + if (thread != null && callerOperation.equals(thread.getRemoteOperation())) { final int threadIndex = thread.getThreadIndex(); thread.setRunning(false); startWorker(threadIndex); @@ -245,8 +248,12 @@ public class SynchronizationService extends Service implements OnRemoteOperation Bundle data = msg.getData(); synchronized (serviceWeakRef.get().runningThreadLock) { - serviceWeakRef.get() - .threadPool[data.getInt("thread index")].setRunning(data.getBoolean("mThreadWorkingState")); + + final SyncThreadHolder threadHolder = serviceWeakRef.get() + .threadPool[data.getInt("thread index")]; + if (threadHolder != null) { + threadHolder.setRunning(data.getBoolean("mThreadWorkingState")); + } } } } -- GitLab