diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index aaf803c70f..156decf2a1 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -443,7 +443,7 @@ export default class Synchronizer { const previousE2EE = localInfo.e2ee; logger.info('Sync target info differs between local and remote - merging infos: ', newInfo.toObject()); - await this.lockHandler().acquireLock(LockType.Exclusive, this.appType_, this.clientId_); + await this.lockHandler().acquireLock(LockType.Exclusive, this.appType_, this.clientId_, { clearExistingSyncLocksFromTheSameClient: true }); await uploadSyncInfo(this.api(), newInfo); await saveLocalSyncInfo(newInfo); await this.lockHandler().releaseLock(LockType.Exclusive, this.appType_, this.clientId_); diff --git a/packages/lib/services/synchronizer/LockHandler.ts b/packages/lib/services/synchronizer/LockHandler.ts index 1ad3f8ab8a..21b915c97c 100644 --- a/packages/lib/services/synchronizer/LockHandler.ts +++ b/packages/lib/services/synchronizer/LockHandler.ts @@ -18,6 +18,29 @@ export interface Lock { updatedTime?: number; } +export interface AcquireLockOptions { + // In theory, a client that tries to acquire an exclusive lock shouldn't + // also have a sync lock. It can however happen when the app is closed + // before the end of the sync process, and then the user tries to upgrade + // the sync target. + // + // So maybe we could always automatically clear the sync locks (that belongs + // to the current client) when acquiring an exclusive lock, but to be safe + // we make the behaviour explicit via this option. It is used for example + // when migrating a sync target. + // + // https://discourse.joplinapp.org/t/error-upgrading-to-2-3-3/19549/4?u=laurent + clearExistingSyncLocksFromTheSameClient?: boolean; + timeoutMs?: number; +} + +function defaultAcquireLockOptions(): AcquireLockOptions { + return { + clearExistingSyncLocksFromTheSameClient: false, + timeoutMs: 0, + }; +} + interface RefreshTimer { id: any; inProgress: boolean; @@ -198,7 +221,7 @@ export default class LockHandler { return `(${lock.clientType} #${lock.clientId})`; } - private async acquireExclusiveLock(clientType: string, clientId: string, timeoutMs: number = 0): Promise { + private async acquireExclusiveLock(clientType: string, clientId: string, options: AcquireLockOptions = null): Promise { // The logic to acquire an exclusive lock, while avoiding race conditions is as follow: // // - Check if there is a lock file present @@ -209,13 +232,18 @@ export default class LockHandler { // // -If there is no lock file, create one with my identifier and try the whole cycle again to avoid race condition (re-check that the lock file is really mine)-. + options = { + ...defaultAcquireLockOptions(), + ...options, + }; + const startTime = Date.now(); async function waitForTimeout() { - if (!timeoutMs) return false; + if (!options.timeoutMs) return false; const elapsed = Date.now() - startTime; - if (timeoutMs && elapsed < timeoutMs) { + if (options.timeoutMs && elapsed < options.timeoutMs) { await time.sleep(2); return true; } @@ -230,8 +258,12 @@ export default class LockHandler { ]); if (activeSyncLock) { - if (await waitForTimeout()) continue; - throw new JoplinError(`Cannot acquire exclusive lock because the following clients have a sync lock on the target: ${this.lockToClientString(activeSyncLock)}`, 'hasSyncLock'); + if (options.clearExistingSyncLocksFromTheSameClient && activeSyncLock.clientId === clientId && activeSyncLock.clientType === clientType) { + await this.releaseLock(LockType.Sync, clientType, clientId); + } else { + if (await waitForTimeout()) continue; + throw new JoplinError(`Cannot acquire exclusive lock because the following clients have a sync lock on the target: ${this.lockToClientString(activeSyncLock)}`, 'hasSyncLock'); + } } if (activeExclusiveLock) { @@ -336,17 +368,22 @@ export default class LockHandler { delete this.refreshTimers_[handle]; } - async acquireLock(lockType: LockType, clientType: string, clientId: string, timeoutMs: number = 0): Promise { + public async acquireLock(lockType: LockType, clientType: string, clientId: string, options: AcquireLockOptions = null): Promise { + options = { + ...defaultAcquireLockOptions(), + ...options, + }; + if (lockType === LockType.Sync) { return this.acquireSyncLock(clientType, clientId); } else if (lockType === LockType.Exclusive) { - return this.acquireExclusiveLock(clientType, clientId, timeoutMs); + return this.acquireExclusiveLock(clientType, clientId, options); } else { throw new Error(`Invalid lock type: ${lockType}`); } } - async releaseLock(lockType: LockType, clientType: string, clientId: string) { + public async releaseLock(lockType: LockType, clientType: string, clientId: string) { await this.api_.delete(this.lockFilePath({ type: lockType, clientType: clientType, diff --git a/packages/lib/services/synchronizer/MigrationHandler.ts b/packages/lib/services/synchronizer/MigrationHandler.ts index 6ae366c46b..e8f0e43e4b 100644 --- a/packages/lib/services/synchronizer/MigrationHandler.ts +++ b/packages/lib/services/synchronizer/MigrationHandler.ts @@ -106,7 +106,11 @@ export default class MigrationHandler extends BaseService { } this.logger().info('MigrationHandler: Acquiring exclusive lock'); - const exclusiveLock = await this.lockHandler_.acquireLock(LockType.Exclusive, this.clientType_, this.clientId_, 1000 * 30); + const exclusiveLock = await this.lockHandler_.acquireLock(LockType.Exclusive, this.clientType_, this.clientId_, { + clearExistingSyncLocksFromTheSameClient: true, + timeoutMs: 1000 * 30, + }); + let autoLockError = null; this.lockHandler_.startAutoLockRefresh(exclusiveLock, (error: any) => { autoLockError = error; diff --git a/packages/lib/services/synchronizer/synchronizer_LockHandler.test.ts b/packages/lib/services/synchronizer/synchronizer_LockHandler.test.ts index 63a24c6407..8b5f2eaac8 100644 --- a/packages/lib/services/synchronizer/synchronizer_LockHandler.test.ts +++ b/packages/lib/services/synchronizer/synchronizer_LockHandler.test.ts @@ -149,6 +149,23 @@ describe('synchronizer_LockHandler', function() { } })); + it('should ignore locks by same client when trying to acquire exclusive lock', (async () => { + const lockHandler = newLockHandler(); + + await lockHandler.acquireLock(LockType.Sync, 'desktop', '111'); + + await expectThrow(async () => { + await lockHandler.acquireLock(LockType.Exclusive, 'desktop', '111', { clearExistingSyncLocksFromTheSameClient: false }); + }, 'hasSyncLock'); + + await expectNotThrow(async () => { + await lockHandler.acquireLock(LockType.Exclusive, 'desktop', '111', { clearExistingSyncLocksFromTheSameClient: true }); + }); + + const activeLock = await lockHandler.activeLock(LockType.Exclusive); + expect(activeLock.clientId).toBe('111'); + })); + // it('should not have race conditions', (async () => { // const lockHandler = newLockHandler();