From cfb9b655ffbe785e8a3947861dace842ceb79a6a Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Fri, 27 Jan 2023 09:45:58 +0100 Subject: [PATCH 1/2] bump nextcloud android library to v1.13.0 --- app/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/build.gradle b/app/build.gradle index a522a752..eefe18c4 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -78,7 +78,7 @@ android { dependencies { - implementation 'com.github.nextcloud:android-library:2.12.1' + implementation 'com.github.nextcloud:android-library:2.13.0' implementation "commons-httpclient:commons-httpclient:3.1@jar" implementation fileTree(include: ['*.jar'], dir: 'libs') api 'androidx.annotation:annotation:1.3.0' -- GitLab From 726684919e6440808aac38ee10977656c26b3ba3 Mon Sep 17 00:00:00 2001 From: Vincent Bourgmayer Date: Fri, 27 Jan 2023 09:05:06 +0000 Subject: [PATCH 2/2] prevent file deletion due to concurrency issue --- .../FileObservers/FileEventListener.java | 10 +-- .../contentScanner/LocalContentScanner.java | 8 +-- .../contentScanner/RemoteContentScanner.java | 5 ++ .../e/drive/models/SyncedFileState.java | 8 ++- .../drive/operations/UploadFileOperation.java | 2 +- .../e/drive/services/ObserverService.java | 62 +++++++++++-------- .../services/SynchronizationService.java | 36 ++++++++++- .../SynchronizationServiceConnection.java | 43 ++++++++++--- 8 files changed, 126 insertions(+), 48 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/FileObservers/FileEventListener.java b/app/src/main/java/foundation/e/drive/FileObservers/FileEventListener.java index 91463c3c..57c342c8 100644 --- a/app/src/main/java/foundation/e/drive/FileObservers/FileEventListener.java +++ b/app/src/main/java/foundation/e/drive/FileObservers/FileEventListener.java @@ -103,7 +103,7 @@ public class FileEventListener { */ private void sendSyncRequestToSynchronizationService(SyncRequest request) { Timber.d("Sending a SyncRequest for %s", request.getSyncedFileState().getName()); - if (serviceConnection.isBoundToSynchronizationService()) { + if (serviceConnection.isBound()) { serviceConnection.getSynchronizationService().queueSyncRequest(request); serviceConnection.getSynchronizationService().startSynchronization(); }else{ @@ -233,14 +233,10 @@ public class FileEventListener { } public void unbindFromSynchronizationService(){ - if (serviceConnection.isBoundToSynchronizationService()) - appContext.unbindService(serviceConnection); - else - Timber.w("Not bound to SynchronizationService: can't unbind."); + serviceConnection.unBind(appContext); } public void bindToSynchronizationService(){ - final Intent SynchronizationServiceIntent = new Intent(appContext, SynchronizationService.class); - appContext.bindService(SynchronizationServiceIntent, serviceConnection, Context.BIND_AUTO_CREATE); + serviceConnection.bindService(appContext); } } diff --git a/app/src/main/java/foundation/e/drive/contentScanner/LocalContentScanner.java b/app/src/main/java/foundation/e/drive/contentScanner/LocalContentScanner.java index 0187ac57..65ad9d4e 100644 --- a/app/src/main/java/foundation/e/drive/contentScanner/LocalContentScanner.java +++ b/app/src/main/java/foundation/e/drive/contentScanner/LocalContentScanner.java @@ -34,9 +34,7 @@ public class LocalContentScanner extends AbstractContentScanner{ @Override protected void onMissingFile(SyncedFileState fileState) { - if (!fileState.hasBeenSynchronizedOnce()) { - return; - } + if (!fileState.hasBeenSynchronizedOnce()) return; final File file = new File(fileState.getLocalPath()); @@ -89,6 +87,4 @@ public class LocalContentScanner extends AbstractContentScanner{ final String filePath = CommonUtils.getLocalPath(file); return fileState.getLocalPath().equals(filePath); } -} - - +} \ No newline at end of file diff --git a/app/src/main/java/foundation/e/drive/contentScanner/RemoteContentScanner.java b/app/src/main/java/foundation/e/drive/contentScanner/RemoteContentScanner.java index 342d9505..9ff7c18c 100644 --- a/app/src/main/java/foundation/e/drive/contentScanner/RemoteContentScanner.java +++ b/app/src/main/java/foundation/e/drive/contentScanner/RemoteContentScanner.java @@ -82,6 +82,11 @@ public class RemoteContentScanner extends AbstractContentScanner { @Override protected void onMissingFile(SyncedFileState fileState) { + if (!fileState.hasBeenSynchronizedOnce()) { + return; + } + + Timber.d("Add local deletion request for file: %s", fileState.getLocalPath()); this.syncRequests.put(fileState.getId(), new SyncRequest(fileState, LOCAL_DELETE)); } diff --git a/app/src/main/java/foundation/e/drive/models/SyncedFileState.java b/app/src/main/java/foundation/e/drive/models/SyncedFileState.java index 38ee3331..952fafbe 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncedFileState.java +++ b/app/src/main/java/foundation/e/drive/models/SyncedFileState.java @@ -15,8 +15,14 @@ import android.os.Parcelable; /** * @author Vincent Bourgmayer * Describe a file state which will be Synchronized (= Synced) or which has already been synced one times + * + * 1. file state: no etag & LM <=0 : new local file discovered + * todo: shouldn't it be more case 2 below ? would break lot of codes + * 2. filestate: no etag but LM > 0 : ??? + * When does it happens ?, and what does it correspond to ? what side effect of such case ? + * 3. fileState: etag but LM <= 0 : new remote file discovered + * 4. fileState: etag & LM > 0: file synchronized once */ - public class SyncedFileState implements Parcelable { public static final int NOT_SCANNABLE=0; public static final int ECLOUD_SCANNABLE=1; diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 5e30d2e6..caa1874b 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -133,7 +133,7 @@ public class UploadFileOperation extends RemoteOperation { //If file already up-to-date & synced if (syncedState.isLastEtagStored() && syncedState.getLocalLastModified() == file.lastModified()) { - Timber.d("Synchronization conflict because: last modified from DB(%s) and from file (%s) are different ", syncedState.getLocalLastModified(), file.lastModified()); + Timber.d("Synchronization conflict because: last modified from DB(%s) and from file (%s) are the same ", syncedState.getLocalLastModified(), file.lastModified()); return ResultCode.SYNC_CONFLICT; } diff --git a/app/src/main/java/foundation/e/drive/services/ObserverService.java b/app/src/main/java/foundation/e/drive/services/ObserverService.java index 3d201f7b..418b28df 100644 --- a/app/src/main/java/foundation/e/drive/services/ObserverService.java +++ b/app/src/main/java/foundation/e/drive/services/ObserverService.java @@ -15,6 +15,7 @@ import static foundation.e.drive.utils.AppConstants.INITIALIZATION_HAS_BEEN_DONE import android.accounts.Account; import android.accounts.AccountManager; import android.app.Service; +import android.content.ComponentName; import android.content.Context; import android.content.Intent; import android.content.SharedPreferences; @@ -29,23 +30,19 @@ import com.owncloud.android.lib.common.OwnCloudClient; import com.owncloud.android.lib.common.operations.OnRemoteOperationListener; import com.owncloud.android.lib.common.operations.RemoteOperation; import com.owncloud.android.lib.common.operations.RemoteOperationResult; -import com.owncloud.android.lib.resources.files.FileUtils; import com.owncloud.android.lib.resources.files.model.RemoteFile; import java.io.File; -import java.io.FileFilter; import java.io.FileOutputStream; import java.util.ArrayList; import java.util.HashMap; import java.util.List; -import java.util.ListIterator; import foundation.e.drive.contentScanner.LocalContentScanner; import foundation.e.drive.contentScanner.LocalFileLister; import foundation.e.drive.contentScanner.RemoteContentScanner; import foundation.e.drive.database.DbHelper; import foundation.e.drive.fileFilters.CrashlogsFileFilter; -import foundation.e.drive.fileFilters.FileFilterFactory; import foundation.e.drive.fileFilters.OnlyFileFilter; import foundation.e.drive.models.SyncRequest; import foundation.e.drive.models.SyncedFileState; @@ -72,32 +69,44 @@ public class ObserverService extends Service implements OnRemoteOperationListene private boolean isWorking = false; private Account mAccount; private HashMap syncRequests; //integer is SyncedFileState id; Parcelable is the operation - private SynchronizationServiceConnection synchronizationServiceConnection = new SynchronizationServiceConnection(); + private SynchronizationServiceConnection synchronizationServiceConnection = new SynchronizationServiceConnection() { + @Override + public void onServiceConnected(ComponentName componentName, IBinder iBinder) { + super.onServiceConnected(componentName, iBinder); + final SharedPreferences prefs = getApplicationContext().getSharedPreferences(AppConstants.SHARED_PREFERENCE_NAME, Context.MODE_PRIVATE); + + if (!checkStartCondition(prefs, forcedSync)) { + stopSelf(); + return; + } + + begin(); + } + }; private Handler handler; private HandlerThread handlerThread; + private boolean forcedSync = false; /* Lifecycle Methods */ @Override public void onDestroy(){ Timber.v("onDestroy()"); - unbindService(synchronizationServiceConnection); - handlerThread.quitSafely(); + synchronizationServiceConnection.unBind(getApplicationContext()); + if (handlerThread != null) handlerThread.quitSafely(); + mSyncedFolders = null; super.onDestroy(); - this.mSyncedFolders = null; } - @Override public void onCreate() { super.onCreate(); Timber.tag(ObserverService.class.getSimpleName()); + synchronizationServiceConnection.bindService(getApplicationContext()); } @Override public int onStartCommand(Intent intent, int flags, int startId) { Timber.i("onStartCommand(%s)", startId); - final Intent SynchronizationServiceIntent = new Intent(this, SynchronizationService.class); - bindService(SynchronizationServiceIntent, synchronizationServiceConnection, Context.BIND_AUTO_CREATE); CommonUtils.setServiceUnCaughtExceptionHandler(this); @@ -106,19 +115,11 @@ public class ObserverService extends Service implements OnRemoteOperationListene final String accountType = prefs.getString(AccountManager.KEY_ACCOUNT_TYPE, ""); this.mAccount = CommonUtils.getAccount(accountName, accountType, AccountManager.get(this)); - final boolean forcedSync = intent != null && DebugCmdReceiver.ACTION_FORCE_SYNC.equals(intent.getAction()); - - if (!checkStartCondition(prefs, forcedSync)) { - return super.onStartCommand(intent, flags, startId); - } + forcedSync = intent != null && DebugCmdReceiver.ACTION_FORCE_SYNC.equals(intent.getAction()); this.syncRequests = new HashMap<>(); - handlerThread = new HandlerThread("syncService_onResponse"); - handlerThread.start(); - handler = new Handler(handlerThread.getLooper()); - begin(); - return START_NOT_STICKY; + return START_STICKY; } /** @@ -170,14 +171,17 @@ public class ObserverService extends Service implements OnRemoteOperationListene return false; } - final boolean meteredNetworkAllowed = CommonUtils.isMeteredNetworkAllowed(mAccount); //check for the case where intent has been launched by initializerService if (!CommonUtils.haveNetworkConnection(this, meteredNetworkAllowed)) { Timber.w("There is no allowed internet connexion."); return false; } - return true; + + final boolean isSyncServicePaused = synchronizationServiceConnection.isBound() + && synchronizationServiceConnection.getSynchronizationService().pauseSync(); + Timber.d("isSyncServicePaused ? %s", isSyncServicePaused); + return isSyncServicePaused; } /* Common methods */ @@ -262,6 +266,9 @@ public class ObserverService extends Service implements OnRemoteOperationListene } try { + handlerThread = new HandlerThread("syncService_onResponse"); + handlerThread.start(); + handler = new Handler(handlerThread.getLooper()); final ListFileRemoteOperation loadOperation = new ListFileRemoteOperation(this.mSyncedFolders, this); loadOperation.execute(client, this, handler); } catch (IllegalArgumentException exception) { @@ -322,6 +329,10 @@ public class ObserverService extends Service implements OnRemoteOperationListene startScan(false); + if (synchronizationServiceConnection.isBound()) { + synchronizationServiceConnection.getSynchronizationService().unPauseSync(); + } + if (!syncRequests.isEmpty()) { Timber.d("syncRequests contains %s", syncRequests.size()); passSyncRequestsToSynchronizationService(); @@ -332,6 +343,7 @@ public class ObserverService extends Service implements OnRemoteOperationListene .putLong(AppConstants.KEY_LAST_SYNC_TIME, System.currentTimeMillis()) .apply(); } + this.isWorking = false; this.stopSelf(); } @@ -340,11 +352,11 @@ public class ObserverService extends Service implements OnRemoteOperationListene * Send all gathered SyncRequest to SynchronizationService */ private void passSyncRequestsToSynchronizationService() { - if (synchronizationServiceConnection.isBoundToSynchronizationService()) { + if (synchronizationServiceConnection.isBound()) { synchronizationServiceConnection.getSynchronizationService().queueSyncRequests(syncRequests.values()); synchronizationServiceConnection.getSynchronizationService().startSynchronization(); } else { - Timber.e("ERROR: impossible to bind ObserverService to SynchronizationService"); + Timber.e("ERROR: binding to SynchronizationService lost"); } } 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 77aa340e..0eaa40ff 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -51,6 +51,7 @@ import timber.log.Timber; */ public class SynchronizationService extends Service implements OnRemoteOperationListener, LocalFileDeleter.LocalFileDeletionListener { private final SynchronizationBinder binder = new SynchronizationBinder(); + private final static long maxPauseTimeInMs = 300000; //5 minutes, might need to be adapted private ConcurrentLinkedDeque syncRequestQueue; private ConcurrentHashMap startedSync; //Integer is thread index (1 to workerAmount) @@ -63,6 +64,8 @@ public class SynchronizationService extends Service implements OnRemoteOperation private NextcloudClient ncClient; private Handler handler; private HandlerThread handlerThread; + private long pauseStartTime = 0L; + @Override public void onCreate() { super.onCreate(); @@ -116,6 +119,35 @@ public class SynchronizationService extends Service implements OnRemoteOperation } + /** + * Try to pause synchronization. + * Can only be paused if no sync is already running and no request queue is started + * @return true if service successfully paused + */ + public boolean pauseSync() { + pauseStartTime = System.currentTimeMillis(); + + final boolean isQueueEmpty = syncRequestQueue.isEmpty(); + final boolean isNoStartedSync = startedSync.values().stream().noneMatch(predicate -> predicate.isRunning()); + + Timber.v("is queue empty ? %s ; is no started sync ? %s", isQueueEmpty, isNoStartedSync); + final boolean isPausable = isQueueEmpty && isNoStartedSync; + if (!isPausable) pauseStartTime = 0L; + return isPausable; + } + + /** + * unpause synchronization + */ + public void unPauseSync() { + pauseStartTime = 0L; + } + + private boolean isPaused() { + if (pauseStartTime == 0L) return false; + return System.currentTimeMillis() - pauseStartTime < maxPauseTimeInMs; + } + /** * Add a SyncRequest into waiting queue if it matches some conditions: * - an equivalent sync Request (same file & same operation type) isn't already running @@ -198,7 +230,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation } private void startWorker(Integer threadIndex){ - if (!canStart(threadIndex)) return; + if (!canStart(threadIndex) || isPaused()) return; final SyncRequest request = this.syncRequestQueue.poll(); //return null if empty if (request == null @@ -210,6 +242,8 @@ public class SynchronizationService extends Service implements OnRemoteOperation final SyncWrapper syncWrapper = new SyncWrapper(request, account, getApplicationContext()); if (request.getOperationType().equals(SyncRequest.Type.LOCAL_DELETE)) { + Timber.v(" starts " + request.getSyncedFileState().getName() + + " local deletion on thread " + threadIndex); final LocalFileDeleter fileDeleter = new LocalFileDeleter(request.getSyncedFileState()); threadPool[threadIndex] = new Thread(fileDeleter.getRunnable( handler, threadIndex, getApplicationContext(), this)); threadPool[threadIndex].start(); diff --git a/app/src/main/java/foundation/e/drive/utils/SynchronizationServiceConnection.java b/app/src/main/java/foundation/e/drive/utils/SynchronizationServiceConnection.java index 5a11626c..370ed76e 100644 --- a/app/src/main/java/foundation/e/drive/utils/SynchronizationServiceConnection.java +++ b/app/src/main/java/foundation/e/drive/utils/SynchronizationServiceConnection.java @@ -9,9 +9,13 @@ package foundation.e.drive.utils; import android.content.ComponentName; +import android.content.Context; +import android.content.Intent; import android.content.ServiceConnection; import android.os.IBinder; +import androidx.annotation.NonNull; + import foundation.e.drive.services.SynchronizationService; import timber.log.Timber; @@ -21,26 +25,28 @@ import timber.log.Timber; public class SynchronizationServiceConnection implements ServiceConnection { private SynchronizationService synchronizationService; - private boolean boundToSynchronizationService = false; + private boolean isBound = false; + private boolean attemptingToBind = false; @Override public void onServiceConnected(ComponentName componentName, IBinder iBinder) { Timber.tag(SynchronizationServiceConnection.class.getSimpleName()) .v("binding to SynchronizationService"); - SynchronizationService.SynchronizationBinder binder = (SynchronizationService.SynchronizationBinder) iBinder; + attemptingToBind = false; + isBound = true; + final SynchronizationService.SynchronizationBinder binder = (SynchronizationService.SynchronizationBinder) iBinder; synchronizationService = binder.getService(); - boundToSynchronizationService = true; } @Override public void onServiceDisconnected(ComponentName componentName) { Timber.tag(SynchronizationServiceConnection.class.getSimpleName()) - .v("Unbinding from SynchronizationService"); - boundToSynchronizationService = false; + .v("onServiceDisconnected()"); + isBound = false; } - public boolean isBoundToSynchronizationService() { - return boundToSynchronizationService; + public boolean isBound() { + return isBound; } /** @@ -50,4 +56,27 @@ public class SynchronizationServiceConnection implements ServiceConnection { public SynchronizationService getSynchronizationService() { return synchronizationService; } + + /** + * Sources: https://stackoverflow.com/questions/8614335/android-how-to-safely-unbind-a-service + * @param context + */ + public void unBind(@NonNull Context context) { + attemptingToBind = false; + if (isBound) { + context.unbindService(this); + isBound = false; + } + } + + /** + * Sources: https://stackoverflow.com/questions/8614335/android-how-to-safely-unbind-a-service + * @param context + */ + public void bindService(@NonNull Context context) { + if (!attemptingToBind) { + attemptingToBind = true; + context.bindService(new Intent(context, SynchronizationService.class), this, Context.BIND_AUTO_CREATE); + } + } } -- GitLab