diff --git a/packages/netlify-cms-backend-github/src/implementation.js b/packages/netlify-cms-backend-github/src/implementation.js index dbd05e48..f4f9123e 100644 --- a/packages/netlify-cms-backend-github/src/implementation.js +++ b/packages/netlify-cms-backend-github/src/implementation.js @@ -2,6 +2,7 @@ import React from 'react'; import trimStart from 'lodash/trimStart'; import semaphore from 'semaphore'; import { stripIndent } from 'common-tags'; +import { asyncLock } from 'netlify-cms-lib-util'; import AuthenticationPage from './AuthenticationPage'; import API from './API'; import GraphQLAPI from './GraphQLAPI'; @@ -67,6 +68,21 @@ export default class GitHub { this.token = ''; this.squash_merges = config.getIn(['backend', 'squash_merges']); this.use_graphql = config.getIn(['backend', 'use_graphql']); + this.lock = asyncLock(); + } + + async runWithLock(func, message) { + try { + const acquired = await this.lock.acquire(); + if (!acquired) { + console.warn(message); + } + + const result = await func(); + return result; + } finally { + this.lock.release(); + } } authComponent() { @@ -281,7 +297,11 @@ export default class GitHub { } persistEntry(entry, mediaFiles = [], options = {}) { - return this.api.persistFiles(entry, mediaFiles, options); + // persistEntry is a transactional operation + return this.runWithLock( + () => this.api.persistFiles(entry, mediaFiles, options), + 'Failed to acquire persist entry lock', + ); } async persistMedia(mediaFile, options = {}) { @@ -394,13 +414,22 @@ export default class GitHub { } updateUnpublishedEntryStatus(collection, slug, newStatus) { - return this.api.updateUnpublishedEntryStatus(collection, slug, newStatus); + // updateUnpublishedEntryStatus is a transactional operation + return this.runWithLock( + () => this.api.updateUnpublishedEntryStatus(collection, slug, newStatus), + 'Failed to acquire update entry status lock', + ); } deleteUnpublishedEntry(collection, slug) { return this.api.deleteUnpublishedEntry(collection, slug); } + publishUnpublishedEntry(collection, slug) { - return this.api.publishUnpublishedEntry(collection, slug); + // publishUnpublishedEntry is a transactional operation + return this.runWithLock( + () => this.api.publishUnpublishedEntry(collection, slug), + 'Failed to acquire publish entry lock', + ); } } diff --git a/packages/netlify-cms-lib-util/src/__tests__/asyncLock.spec.js b/packages/netlify-cms-lib-util/src/__tests__/asyncLock.spec.js new file mode 100644 index 00000000..69a780fe --- /dev/null +++ b/packages/netlify-cms-lib-util/src/__tests__/asyncLock.spec.js @@ -0,0 +1,85 @@ +import { asyncLock } from '../asyncLock'; + +jest.useFakeTimers(); +jest.spyOn(console, 'warn').mockImplementation(() => {}); + +describe('asyncLock', () => { + it('should be able to acquire a new lock', async () => { + const lock = asyncLock(); + + const acquired = await lock.acquire(); + + expect(acquired).toBe(true); + }); + + it('should not be able to acquire an acquired lock', async () => { + const lock = asyncLock(); + await lock.acquire(); + + const promise = lock.acquire(); + + // advance by default lock timeout + jest.advanceTimersByTime(15000); + + const acquired = await promise; + + expect(acquired).toBe(false); + }); + + it('should be able to acquire an acquired lock that was released', async () => { + const lock = asyncLock(); + await lock.acquire(); + + const promise = lock.acquire(); + + // release the lock in the "future" + setTimeout(() => lock.release(), 100); + + // advance to the time where the lock will be released + jest.advanceTimersByTime(100); + + const acquired = await promise; + + expect(acquired).toBe(true); + }); + + it('should accept a timeout for acquire', async () => { + const lock = asyncLock(); + await lock.acquire(); + + const promise = lock.acquire(50); + + /// advance by lock timeout + jest.advanceTimersByTime(50); + + const acquired = await promise; + + expect(acquired).toBe(false); + }); + + it('should be able to re-acquire a lock after a timeout', async () => { + const lock = asyncLock(); + await lock.acquire(); + + const promise = lock.acquire(); + + // advance by default lock timeout + jest.advanceTimersByTime(15000); + + let acquired = await promise; + + expect(acquired).toBe(false); + + acquired = await lock.acquire(); + expect(acquired).toBe(true); + }); + + it('should suppress "leave called too many times" error', async () => { + const lock = asyncLock(); + + await expect(() => lock.release()).not.toThrow(); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith('leave called too many times.'); + }); +}); diff --git a/packages/netlify-cms-lib-util/src/asyncLock.js b/packages/netlify-cms-lib-util/src/asyncLock.js new file mode 100644 index 00000000..e2518b04 --- /dev/null +++ b/packages/netlify-cms-lib-util/src/asyncLock.js @@ -0,0 +1,41 @@ +import semaphore from 'semaphore'; + +export const asyncLock = () => { + let lock = semaphore(1); + + const acquire = (timeout = 15000) => { + const promise = new Promise(resolve => { + // this makes sure a caller doesn't gets stuck forever awaiting on the lock + const timeoutId = setTimeout(() => { + // we reset the lock in that case to allow future consumers to use it without being blocked + lock = semaphore(1); + resolve(false); + }, timeout); + + lock.take(() => { + clearTimeout(timeoutId); + resolve(true); + }); + }); + + return promise; + }; + + const release = () => { + try { + // suppress too many calls to leave error + lock.leave(); + } catch (e) { + // calling 'leave' too many times might not be good behavior + // but there is no reason to completely fail on it + if (e.message !== 'leave called too many times.') { + throw e; + } else { + console.warn('leave called too many times.'); + lock = semaphore(1); + } + } + }; + + return { acquire, release }; +}; diff --git a/packages/netlify-cms-lib-util/src/index.js b/packages/netlify-cms-lib-util/src/index.js index 41559eaa..fbf5545c 100644 --- a/packages/netlify-cms-lib-util/src/index.js +++ b/packages/netlify-cms-lib-util/src/index.js @@ -26,6 +26,7 @@ import { } from './backendUtil'; import loadScript from './loadScript'; import getBlobSHA from './getBlobSHA'; +import { asyncLock } from './asyncLock'; export const NetlifyCmsLibUtil = { APIError, @@ -77,4 +78,5 @@ export { responseParser, loadScript, getBlobSHA, + asyncLock, };