diff --git a/package.json b/package.json index a12be1540..1b3c503ca 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "@fastify/multipart": "^8.3.0 || ^9.0.1", "fastify": "^4.28.1 || ^5.1.0", "fs-extra": "^11.2.0", - "lockfile": "^1.0.4" + "proper-lockfile": "^4.1.2" }, "devDependencies": { "@babel/core": "^7.12.3", @@ -56,7 +56,7 @@ "@tsconfig/node14": "^14.1.2", "@types/fs-extra": "^11.0.4", "@types/jest": "^29.5.12", - "@types/lockfile": "^1.0.4", + "@types/proper-lockfile": "^4.1.4", "@types/touch": "^3.1.5", "babel-jest": "^29.7.0", "concurrently": "^9.1.0", diff --git a/packages/node-renderer/src/shared/locks.ts b/packages/node-renderer/src/shared/locks.ts index 32956d308..21a98f16a 100644 --- a/packages/node-renderer/src/shared/locks.ts +++ b/packages/node-renderer/src/shared/locks.ts @@ -1,80 +1,55 @@ -import lockfile, { Options } from 'lockfile'; -import { promisify } from 'util'; +import { lock as lockLib, LockOptions } from 'proper-lockfile'; import debug from './debug'; import log from './log'; import { delay, workerIdLabel } from './utils'; -const lockfileLockAsync = promisify(lockfile.lock); -const lockfileUnlockAsync = promisify(lockfile.unlock); - const TEST_LOCKFILE_THREADING = false; -// See definitions here: https://github.com/npm/lockfile/blob/master/README.md#options -/* - * A number of milliseconds to wait for locks to expire before giving up. Only used by - * lockFile.lock. Poll for opts.wait ms. If the lock is not cleared by the time the wait expires, - * then it returns with the original error. - */ -const LOCKFILE_WAIT = 3000; - -/* - * When using opts.wait, this is the period in ms in which it polls to check if the lock has - * expired. Defaults to 100. - */ -const LOCKFILE_POLL_PERIOD = 300; // defaults to 100 - +// See definitions here: https://www.npmjs.com/package/proper-lockfile?activeTab=readme#lockfile-options +// and https://www.npmjs.com/package/retry#retryoperationoptions for retries /* * A number of milliseconds before locks are considered to have expired. */ const LOCKFILE_STALE = 20000; /* - * Used by lock and lockSync. Retry n number of times before giving up. + * The number of retries. */ const LOCKFILE_RETRIES = 45; /* - * Used by lock. Wait n milliseconds before retrying. + * The number of milliseconds before starting the first retry. */ -const LOCKFILE_RETRY_WAIT = 300; +const LOCKFILE_RETRY_MIN_TIMEOUT = 300; -const lockfileOptions = { - wait: LOCKFILE_WAIT, - retryWait: LOCKFILE_RETRY_WAIT, - retries: LOCKFILE_RETRIES, +const lockfileOptions: LockOptions = { + // so the argument doesn't have to be an existing file + realpath: false, + retries: { + retries: LOCKFILE_RETRIES, + minTimeout: LOCKFILE_RETRY_MIN_TIMEOUT, + maxTimeout: 100 * LOCKFILE_RETRY_MIN_TIMEOUT, + randomize: true, + }, stale: LOCKFILE_STALE, - pollPeriod: LOCKFILE_POLL_PERIOD, }; -export async function unlock(lockfileName: string) { - debug('Worker %s: About to unlock %s', workerIdLabel(), lockfileName); - log.info('Worker %s: About to unlock %s', workerIdLabel(), lockfileName); - - await lockfileUnlockAsync(lockfileName); -} - -type LockResult = { - lockfileName: string; -} & ( - | { - wasLockAcquired: true; - errorMessage: null; - } - | { - wasLockAcquired: false; - errorMessage: Error; - } -); +type LockResult = | { + wasLockAcquired: true; + release: () => Promise; +} | { + wasLockAcquired: false; + error: Error; +}; export async function lock(filename: string): Promise { - const lockfileName = `${filename}.lock`; const workerId = workerIdLabel(); try { - debug('Worker %s: About to request lock %s', workerId, lockfileName); - log.info('Worker %s: About to request lock %s', workerId, lockfileName); - await lockfileLockAsync(lockfileName, lockfileOptions); + debug('Worker %s: About to request lock %s', workerId, filename); + log.info('Worker %s: About to request lock %s', workerId, filename); + const releaseLib = await lockLib(filename, lockfileOptions); // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- the const may be changed to test threading if (TEST_LOCKFILE_THREADING) { @@ -82,10 +57,17 @@ export async function lock(filename: string): Promise { await delay(5000); debug('Worker %i: handleNewBundleProvided done sleeping 5s', workerId); } - debug('After acquired lock in pid', lockfileName); + debug('After acquired lock in pid', filename); + return { + wasLockAcquired: true, + release: () => { + debug('Worker %s: About to unlock %s', workerIdLabel(), filename); + log.info('Worker %s: About to unlock %s', workerIdLabel(), filename); + return releaseLib(); + }, + }; } catch (error) { - log.info('Worker %s: Failed to acquire lock %s, error %s', workerId, lockfileName, error); - return { lockfileName, wasLockAcquired: false, errorMessage: error as Error }; + log.info('Worker %s: Failed to acquire lock %s, error %s', workerId, filename, error); + return { wasLockAcquired: false, error: error as Error }; } - return { lockfileName, wasLockAcquired: true, errorMessage: null }; } diff --git a/packages/node-renderer/src/worker.ts b/packages/node-renderer/src/worker.ts index 83dd4c337..4ccd464b2 100644 --- a/packages/node-renderer/src/worker.ts +++ b/packages/node-renderer/src/worker.ts @@ -26,7 +26,7 @@ import { Asset, } from './shared/utils'; import * as errorReporter from './shared/errorReporter'; -import { lock, unlock } from './shared/locks'; +import { lock } from './shared/locks'; import { startSsrRequestOptions, trace } from './shared/tracing'; // Uncomment the below for testing timeouts: @@ -240,20 +240,17 @@ export default function run(config: Partial) { if (!(await requestPrechecks(req, res))) { return; } - let lockAcquired = false; - let lockfileName: string | undefined; const assets: Asset[] = Object.values(req.body).filter(isAsset); const assetsDescription = JSON.stringify(assets.map((asset) => asset.filename)); const taskDescription = `Uploading files ${assetsDescription} to ${bundlePath}`; + const lockfileName = 'transferring-assets'; + // lock already catches errors internally, so it's safe to call without try/catch + const lockResult = await lock(lockfileName); try { - const { lockfileName: name, wasLockAcquired, errorMessage } = await lock('transferring-assets'); - lockfileName = name; - lockAcquired = wasLockAcquired; - - if (!wasLockAcquired) { + if (!lockResult.wasLockAcquired) { const msg = formatExceptionMessage( taskDescription, - errorMessage, + lockResult.error, `Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`, ); await setResponse(errorResponseResult(msg), res); @@ -280,11 +277,9 @@ export default function run(config: Partial) { } } } finally { - if (lockAcquired) { + if (lockResult.wasLockAcquired) { try { - if (lockfileName) { - await unlock(lockfileName); - } + await lockResult.release(); } catch (error) { log.warn({ msg: `Error unlocking ${lockfileName} from worker ${workerIdLabel()}`, diff --git a/packages/node-renderer/src/worker/handleRenderRequest.ts b/packages/node-renderer/src/worker/handleRenderRequest.ts index 3547d7ba0..b59ba686b 100644 --- a/packages/node-renderer/src/worker/handleRenderRequest.ts +++ b/packages/node-renderer/src/worker/handleRenderRequest.ts @@ -7,7 +7,7 @@ import cluster from 'cluster'; import path from 'path'; -import { lock, unlock } from '../shared/locks'; +import { lock } from '../shared/locks'; import fileExistsAsync from '../shared/fileExistsAsync'; import log from '../shared/log'; import { @@ -84,18 +84,14 @@ async function handleNewBundleProvided( ): Promise { log.info('Worker received new bundle: %s', bundleFilePathPerTimestamp); - let lockAcquired = false; - let lockfileName: string | undefined; + // lock already catches errors internally, so it's safe to call without try/catch + const lockResult = await lock(bundleFilePathPerTimestamp); try { - const { lockfileName: name, wasLockAcquired, errorMessage } = await lock(bundleFilePathPerTimestamp); - lockfileName = name; - lockAcquired = wasLockAcquired; - - if (!wasLockAcquired) { + if (!lockResult.wasLockAcquired) { const msg = formatExceptionMessage( renderingRequest, - errorMessage, - `Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`, + lockResult.error, + `Failed to acquire lock ${bundleFilePathPerTimestamp}. Worker: ${workerIdLabel()}.`, ); return Promise.resolve(errorResponseResult(msg)); } @@ -143,17 +139,15 @@ to ${bundleFilePathPerTimestamp})`, return Promise.resolve(errorResponseResult(msg)); } } finally { - if (lockAcquired) { - log.info('About to unlock %s from worker %i', lockfileName, workerIdLabel()); + if (lockResult.wasLockAcquired) { + log.info('About to unlock %s from worker %i', bundleFilePathPerTimestamp, workerIdLabel()); try { - if (lockfileName) { - await unlock(lockfileName); - } + await lockResult.release(); } catch (error) { const msg = formatExceptionMessage( renderingRequest, error, - `Error unlocking ${lockfileName} from worker ${workerIdLabel()}.`, + `Error unlocking ${bundleFilePathPerTimestamp} from worker ${workerIdLabel()}.`, ); log.warn(msg); } diff --git a/packages/node-renderer/tests/handleRenderRequest.test.ts b/packages/node-renderer/tests/handleRenderRequest.test.ts index fe151fd6b..f2a9ad560 100644 --- a/packages/node-renderer/tests/handleRenderRequest.test.ts +++ b/packages/node-renderer/tests/handleRenderRequest.test.ts @@ -1,17 +1,17 @@ import path from 'path'; import touch from 'touch'; -import lockfile from 'lockfile'; +import { lockSync } from 'proper-lockfile'; import { - createVmBundle, - uploadedBundlePath, - createUploadedBundle, - resetForTest, BUNDLE_TIMESTAMP, + createUploadedBundle, + createVmBundle, lockfilePath, + resetForTest, + uploadedBundlePath, } from './helper'; import { getVmBundleFilePath } from '../src/worker/vm'; import handleRenderRequest from '../src/worker/handleRenderRequest'; -import { delay, Asset } from '../src/shared/utils'; +import { Asset, delay } from '../src/shared/utils'; const testName = 'handleRenderRequest'; const uploadedBundleForTest = (): Asset => ({ @@ -79,12 +79,6 @@ describe(testName, () => { }); test('If lockfile exists, and is stale', async () => { - // We're using a lockfile with an artificially old date, - // so make it use that instead of ctime. - // Probably you should never do this in production! - // @ts-expect-error Not allowed by the types - lockfile.filetime = 'mtime'; - expect.assertions(2); touch.sync(lockfilePathForTest(), { time: '1979-07-01T19:10:00.000Z' }); await createUploadedBundleForTest(); @@ -103,16 +97,17 @@ describe(testName, () => { expect.assertions(2); await createUploadedBundleForTest(); - const lockfileOptions = { pollPeriod: 100, stale: 10000 }; - lockfile.lockSync(lockfilePathForTest(), lockfileOptions); + const release = lockSync( + lockfilePathForTest(), + { stale: 10000, realpath: false }, + ); await delay(5); console.log('TEST building VM from sleep'); await createVmBundleForTest(); console.log('TEST DONE building VM from sleep'); - lockfile.unlock(lockfilePathForTest(), (err) => { - console.log('TEST unlocked lockfile', err); - }); + release(); + console.log('TEST unlocked lockfile'); const result = await handleRenderRequest({ renderingRequest: 'ReactOnRails.dummy', diff --git a/yarn.lock b/yarn.lock index 6e32bc0f4..0eb082083 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1789,11 +1789,6 @@ dependencies: "@types/node" "*" -"@types/lockfile@^1.0.4": - version "1.0.4" - resolved "https://registry.yarnpkg.com/@types/lockfile/-/lockfile-1.0.4.tgz#9d6a6d1b6dbd4853cecc7f334bc53ea0ff363b8e" - integrity sha512-Q8oFIHJHr+htLrTXN2FuZfg+WXVHQRwU/hC2GpUu+Q8e3FUM9EDkS2pE3R2AO1ZGu56f479ybdMCNF1DAu8cAQ== - "@types/mime@^1": version "1.3.5" resolved "https://registry.yarnpkg.com/@types/mime/-/mime-1.3.5.tgz#1ef302e01cf7d2b5a0fa526790c9123bf1d06690" @@ -1811,6 +1806,13 @@ resolved "https://registry.yarnpkg.com/@types/parse-json/-/parse-json-4.0.2.tgz#5950e50960793055845e956c427fc2b0d70c5239" integrity sha512-dISoDXWWQwUquiKsyZ4Ng+HX2KsPL7LyHKHQwgGFEA3IaKac4Obd+h2a/a6waisAoepJlBcx9paWqjA8/HVjCw== +"@types/proper-lockfile@^4.1.4": + version "4.1.4" + resolved "https://registry.yarnpkg.com/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz#cd9fab92bdb04730c1ada542c356f03620f84008" + integrity sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ== + dependencies: + "@types/retry" "*" + "@types/qs@*": version "6.9.15" resolved "https://registry.yarnpkg.com/@types/qs/-/qs-6.9.15.tgz#adde8a060ec9c305a82de1babc1056e73bd64dce" @@ -1821,6 +1823,11 @@ resolved "https://registry.yarnpkg.com/@types/range-parser/-/range-parser-1.2.7.tgz#50ae4353eaaddc04044279812f52c8c65857dbcb" integrity sha512-hKormJbkJqzQGhziax5PItDUTMAM9uE2XXQmM37dyd4hVM+5aVl7oVxMVUiVQn2oCQFN/LKCZdvSM0pFRqbSmQ== +"@types/retry@*": + version "0.12.5" + resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.5.tgz#f090ff4bd8d2e5b940ff270ab39fd5ca1834a07e" + integrity sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw== + "@types/semver@^7.3.12": version "7.5.8" resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.8.tgz#8268a8c57a3e4abd25c165ecd36237db7948a55e" @@ -5831,13 +5838,6 @@ locate-path@^6.0.0: dependencies: p-locate "^5.0.0" -lockfile@^1.0.4: - version "1.0.4" - resolved "https://registry.yarnpkg.com/lockfile/-/lockfile-1.0.4.tgz#07f819d25ae48f87e538e6578b6964a4981a5609" - integrity sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA== - dependencies: - signal-exit "^3.0.2" - lodash.capitalize@^4.2.1: version "4.2.1" resolved "https://registry.yarnpkg.com/lodash.capitalize/-/lodash.capitalize-4.2.1.tgz#f826c9b4e2a8511d84e3aca29db05e1a4f3b72a9" @@ -6789,6 +6789,15 @@ prop-types@^15.8.1: object-assign "^4.1.1" react-is "^16.13.1" +proper-lockfile@^4.1.2: + version "4.1.2" + resolved "https://registry.yarnpkg.com/proper-lockfile/-/proper-lockfile-4.1.2.tgz#c8b9de2af6b2f1601067f98e01ac66baa223141f" + integrity sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA== + dependencies: + graceful-fs "^4.2.4" + retry "^0.12.0" + signal-exit "^3.0.2" + proto-list@~1.2.1: version "1.2.4" resolved "https://registry.yarnpkg.com/proto-list/-/proto-list-1.2.4.tgz#212d5bfe1318306a420f6402b8e26ff39647a849" @@ -7216,6 +7225,11 @@ retry@0.13.1: resolved "https://registry.yarnpkg.com/retry/-/retry-0.13.1.tgz#185b1587acf67919d63b357349e03537b2484658" integrity sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg== +retry@^0.12.0: + version "0.12.0" + resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b" + integrity sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow== + reusify@^1.0.4: version "1.0.4" resolved "https://registry.yarnpkg.com/reusify/-/reusify-1.0.4.tgz#90da382b1e126efc02146e90845a88db12925d76"