Merge pull request #1488 from floccusaddon/refactor/wait-lock

refactor: Move waiting for lock out of adapters into controller
floccusaddon · Dec 20, 2023 · 1e46975 · 1e46975
2 parents 128410a + 8ee5783
commit 1e46975
Show file tree

Hide file tree

Showing 14 changed files with 158 additions and 98 deletions.
diff --git a/_locales/en/messages.json b/_locales/en/messages.json
@@ -671,5 +671,11 @@
   },
   "LabelImportsuccessful": {
     "message": "Successfully imported profile(s)"
+  },
+  "DescriptionSyncinprogress": {
+    "message": "Synchronization in progress."
+  },
+  "DescriptionSyncscheduled": {
+    "message": "This profile will be synced soon. We're either waiting for other devices of yours, or other profiles on this device, to finish syncing."
   }
 }
diff --git a/manifest.json b/manifest.json
@@ -10,13 +10,6 @@
     "128": "icons/logo_128.png"
   },
 
-  "browser_specific_settings": {
-    "gecko": {
-      "id": "floccus@handmadeideas.org",
-      "strict_min_version": "109.0"
-    }
-  },
-
   "default_locale": "en",
 
   "permissions": ["alarms", "bookmarks", "storage", "unlimitedStorage", "tabs", "identity"],
@@ -42,6 +35,6 @@
   },
 
   "background": {
-    "scripts": ["dist/js/background-script.js"]
+    "service_worker": "dist/js/background-script.js"
   }
 }
diff --git a/src/errors/Error.ts b/src/errors/Error.ts
@@ -305,3 +305,11 @@ export class MissingPermissionsError extends FloccusError {
     Object.setPrototypeOf(this, MissingPermissionsError.prototype)
   }
 }
+
+export class ResourceLockedError extends FloccusError {
+  constructor() {
+    super(`E037: Resource is locked`)
+    this.code = 37
+    Object.setPrototypeOf(this, MissingPermissionsError.prototype)
+  }
+}
diff --git a/src/lib/Account.ts b/src/lib/Account.ts
@@ -10,6 +10,7 @@ import { IResource, TLocalTree } from './interfaces/Resource'
 import { Capacitor } from '@capacitor/core'
 import IAccount from './interfaces/Account'
 import Mappings from './Mappings'
+import { ResourceLockedError } from '../errors/Error'
 
 // register Adapters
 AdapterFactory.register('nextcloud-folders', async() => (await import('./adapters/NextcloudBookmarks')).default)
@@ -146,7 +147,7 @@ export default class Account {
 
       Logger.log('Starting sync process for account ' + this.getLabel())
       this.syncing = true
-      await this.setData({ ...this.getData(), syncing: 0.05, error: null })
+      await this.setData({ ...this.getData(), syncing: 0.05, scheduled: false, error: null })
 
       if (!(await this.isInitialized())) {
         await this.init()
@@ -163,7 +164,23 @@ export default class Account {
 
       if (this.server.onSyncStart) {
         const needLock = (strategy || this.getData().strategy) !== 'slave'
-        const status = await this.server.onSyncStart(needLock)
+        let status
+        try {
+          status = await this.server.onSyncStart(needLock)
+        } catch (e) {
+          // Resource locked
+          if (e.code === 37) {
+            await this.setData({ ...this.getData(), error: null, syncing: false, scheduled: true })
+            this.syncing = false
+            Logger.log(
+              'Resource is locked, trying again soon'
+            )
+            await Logger.persist()
+            return
+          } else {
+            throw e
+          }
+        }
         if (status === false) {
           await this.init()
         }
@@ -210,7 +227,7 @@ export default class Account {
       }
       await this.syncProcess.sync()
 
-      await this.setData({ ...this.getData(), syncing: 1 })
+      await this.setData({ ...this.getData(), scheduled: false, syncing: 1 })
 
       // update cache
       if (localResource.constructor.name !== 'LocalTabs') {
@@ -234,6 +251,7 @@ export default class Account {
         ...this.getData(),
         error: null,
         syncing: false,
+        scheduled: false,
         lastSync: Date.now(),
       })
 
@@ -250,6 +268,7 @@ export default class Account {
         ...this.getData(),
         error: message,
         syncing: false,
+        scheduled: false,
       })
       this.syncing = false
       if (this.server.onSyncFail) {

diff --git a/src/lib/adapters/GoogleDrive.ts b/src/lib/adapters/GoogleDrive.ts
@@ -8,7 +8,7 @@ import {
   DecryptionError, FileUnreadableError,
   GoogleDriveAuthenticationError, InterruptedSyncError, MissingPermissionsError,
   NetworkError,
-  OAuthTokenError
+  OAuthTokenError, ResourceLockedError
 } from '../../errors/Error'
 import { OAuth2Client } from '@byteowls/capacitor-oauth2'
 import { Capacitor, CapacitorHttp as Http } from '@capacitor/core'
@@ -191,7 +191,7 @@ export default class GoogleDriveAdapter extends CachingAdapter {
     })
   }
 
-  async onSyncStart() {
+  async onSyncStart(needLock = true) {
     Logger.log('onSyncStart: begin')
 
     if (Capacitor.getPlatform() === 'web') {
@@ -204,31 +204,29 @@ export default class GoogleDriveAdapter extends CachingAdapter {
 
     this.accessToken = await this.getAccessToken(this.server.refreshToken)
 
-    let file
-    let startDate = Date.now()
-    const maxTimeout = LOCK_TIMEOUT
-    const base = 1.25
-    for (let i = 0; Date.now() - startDate < maxTimeout; i++) {
-      const fileList = await this.listFiles('name = ' + "'" + this.server.bookmark_file + "'")
-      file = fileList.files.filter(file => !file.trashed)[0]
-      if (file) {
-        this.fileId = file.id
+    const fileList = await this.listFiles('name = ' + "'" + this.server.bookmark_file + "'")
+    const file = fileList.files.filter(file => !file.trashed)[0]
+    if (file) {
+      this.fileId = file.id
+      if (needLock) {
         const data = await this.getFileMetadata(file.id, 'appProperties')
         if (data.appProperties && data.appProperties.locked && (data.appProperties.locked === true || JSON.parse(data.appProperties.locked))) {
           const lockedDate = JSON.parse(data.appProperties.locked)
-          if (Number.isInteger(lockedDate)) {
-            startDate = lockedDate
+          if (!Number.isInteger(lockedDate)) {
+            throw new ResourceLockedError()
+          }
+          if (Date.now() - lockedDate < LOCK_TIMEOUT) {
+            throw new ResourceLockedError()
           }
-          await this.timeout(base ** i * 1000)
-          continue
         }
       }
-      break
     }
 
     if (file) {
       this.fileId = file.id
-      await this.setLock(this.fileId)
+      if (needLock) {
+        await this.setLock(this.fileId)
+      }
 
       let xmlDocText = await this.downloadFile(this.fileId)
 

diff --git a/src/lib/adapters/NextcloudBookmarks.ts b/src/lib/adapters/NextcloudBookmarks.ts
@@ -22,7 +22,7 @@ import {
   NetworkError,
   ParseResponseError,
   RedirectError,
-  RequestTimeoutError,
+  RequestTimeoutError, ResourceLockedError,
   UnexpectedServerResponseError,
   UnknownCreateTargetError,
   UnknownFolderParentUpdateError,
@@ -137,26 +137,21 @@ export default class NextcloudBookmarksAdapter implements Adapter, BulkImportRes
     })
   }
 
-  async onSyncStart(): Promise<void> {
+  async onSyncStart(needLock = true): Promise<void> {
     if (Capacitor.getPlatform() === 'web') {
       const browser = (await import('../browser-api')).default
       if (!(await browser.permissions.contains({ origins: [this.server.url + '/'] }))) {
         throw new MissingPermissionsError()
       }
     }
 
-    this.canceled = false
-    const startDate = Date.now()
-    const maxTimeout = LOCK_TIMEOUT
-    const base = 1.25
-    for (let i = 0; Date.now() - startDate < maxTimeout; i++) {
-      if (await this.acquireLock()) {
-        break
-      } else {
-        Logger.log('Resource is still locked, trying again in ' + (base ** i) + 's')
-        await this.timeout(base ** i * 1000)
+    if (needLock) {
+      if (!(await this.acquireLock())) {
+        throw new ResourceLockedError()
       }
     }
+
+    this.canceled = false
     this.ended = false
     this.lockingInterval = setInterval(() => !this.ended && this.acquireLock(), LOCK_INTERVAL)
   }

diff --git a/src/lib/adapters/WebDav.ts b/src/lib/adapters/WebDav.ts
@@ -10,7 +10,7 @@ import {
   DecryptionError, FileUnreadableError,
   HttpError, InterruptedSyncError,
   LockFileError, MissingPermissionsError,
-  NetworkError, RedirectError,
+  NetworkError, RedirectError, ResourceLockedError,
   SlashError
 } from '../../errors/Error'
 import { CapacitorHttp as Http } from '@capacitor/core'
@@ -92,20 +92,16 @@ export default class WebDavAdapter extends CachingAdapter {
   }
 
   async obtainLock() {
-    let res
-    let startDate = Date.now()
-    const maxTimeout = LOCK_TIMEOUT
-    const base = 1.25
-    for (let i = 0; Date.now() - startDate < maxTimeout; i++) {
-      res = await this.checkLock()
-      if (res.status === 200) {
-        if (res.headers['Last-Modified']) {
-          const date = new Date(res.headers['Last-Modified'])
-          startDate = date.valueOf()
+    const res = await this.checkLock()
+    if (res.status === 200) {
+      if (res.headers['Last-Modified']) {
+        const date = new Date(res.headers['Last-Modified'])
+        const dateLocked = date.valueOf()
+        if (Date.now() - dateLocked < LOCK_TIMEOUT) {
+          throw new ResourceLockedError()
         }
-        await this.timeout(base ** i * 1000)
-      } else if (res.status !== 200) {
-        break
+      } else {
+        throw new ResourceLockedError()
       }
     }
 

diff --git a/src/lib/browser/BrowserController.js b/src/lib/browser/BrowserController.js
@@ -6,14 +6,9 @@ import Cryptography from '../Crypto'
 import packageJson from '../../../package.json'
 import BrowserAccountStorage from './BrowserAccountStorage'
 import uniqBy from 'lodash/uniqBy'
-
-import PQueue from 'p-queue'
 import Account from '../Account'
+import { STATUS_ALLGOOD, STATUS_DISABLED, STATUS_ERROR, STATUS_SYNCING } from '../interfaces/Controller'
 
-const STATUS_ERROR = Symbol('error')
-const STATUS_SYNCING = Symbol('syncing')
-const STATUS_ALLGOOD = Symbol('allgood')
-const STATUS_DISABLED = Symbol('disabled')
 const INACTIVITY_TIMEOUT = 7 * 1000
 const DEFAULT_SYNC_INTERVAL = 15
 
@@ -29,6 +24,9 @@ class AlarmManager {
       const data = account.getData()
       const lastSync = data.lastSync || 0
       const interval = data.syncInterval || DEFAULT_SYNC_INTERVAL
+      if (data.scheduled) {
+        this.ctl.scheduleSync(accountId)
+      }
       if (
         Date.now() >
         interval * 1000 * 60 + lastSync
@@ -42,8 +40,6 @@ class AlarmManager {
 
 export default class BrowserController {
   constructor() {
-    this.jobs = new PQueue({ concurrency: 1 })
-    this.waiting = {}
     this.schedule = {}
     this.listeners = []
 
@@ -265,22 +261,21 @@ export default class BrowserController {
       return
     }
 
-    if (this.waiting[accountId]) {
-      console.log('Account is already queued to be synced')
+    const status = await this.getStatus()
+    if (status === STATUS_SYNCING) {
+      await account.setData({ ...account.getData(), scheduled: true })
       return
     }
 
-    this.waiting[accountId] = true
-    await account.setData({ ...account.getData(), scheduled: true })
-
-    return this.jobs.add(() => this.syncAccount(accountId))
+    this.syncAccount(accountId)
   }
 
   async scheduleAll() {
     const accounts = await Account.getAllAccounts()
     for (const account of accounts) {
-      this.scheduleSync(account.id)
+      await account.setData({...account.getData(), scheduled: true})
     }
+    this.updateStatus()
   }
 
   async cancelSync(accountId, keepEnabled) {
@@ -294,13 +289,11 @@ export default class BrowserController {
 
   async syncAccount(accountId, strategy) {
     console.log('Called syncAccount ', accountId)
-    this.waiting[accountId] = false
     if (!this.enabled) {
       console.log('Flocccus controller is not enabled. Not syncing.')
       return
     }
     let account = await Account.get(accountId)
-    await account.setData({ ...account.getData(), scheduled: false })
     if (account.getData().syncing) {
       console.log('Account is already syncing. Not triggering another sync.')
       return
@@ -329,14 +322,14 @@ export default class BrowserController {
     }
   }
 
-  async updateBadge() {
+  async getStatus() {
     if (!this.unlocked) {
-      return this.setStatusBadge(STATUS_ERROR)
+      return STATUS_ERROR
     }
     const accounts = await Account.getAllAccounts()
     let overallStatus = accounts.reduce((status, account) => {
       const accData = account.getData()
-      if (status === STATUS_SYNCING || accData.syncing) {
+      if (status === STATUS_SYNCING || accData.syncing || account.syncing) {
         return STATUS_SYNCING
       } else if (status === STATUS_ERROR || (accData.error && !accData.syncing)) {
         return STATUS_ERROR
@@ -351,7 +344,11 @@ export default class BrowserController {
       }
     }
 
-    this.setStatusBadge(overallStatus)
+    return overallStatus
+  }
+
+  async updateBadge() {
+    await this.setStatusBadge(await this.getStatus())
   }
 
   async setStatusBadge(status) {
@@ -375,7 +372,6 @@ export default class BrowserController {
           await acc.setData({
             ...acc.getData(),
             syncing: false,
-            error: false,
           })
         }
       })

diff --git a/src/lib/interfaces/Controller.ts b/src/lib/interfaces/Controller.ts
@@ -9,3 +9,8 @@ export default interface IController {
   getUnlocked():Promise<boolean>;
   onLoad():void;
 }
+
+export const STATUS_ERROR = Symbol('error')
+export const STATUS_SYNCING = Symbol('syncing')
+export const STATUS_ALLGOOD = Symbol('allgood')
+export const STATUS_DISABLED = Symbol('disabled')