Skip to content

WIP Switch from lockfile to proper-lockfile #517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alexeyr-ci wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP Switch from lockfile to proper-lockfile #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
90 changes: 36 additions & 54 deletions packages/node-renderer/src/shared/locks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,91 +1,73 @@
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<string, Options>(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<void>;
} | {
wasLockAcquired: false;
error: Error;
};

export async function lock(filename: string): Promise<LockResult> {
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) {
debug('Worker %i: handleNewBundleProvided sleeping 5s', workerId);
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 };
}
21 changes: 8 additions & 13 deletions packages/node-renderer/src/worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -240,20 +240,17 @@ export default function run(config: Partial<Config>) {
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);
Copy link
Contributor Author

@alexeyr-ci alexeyr-ci Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could keep the call inside try/catch, but that complicates code unnecessarily.

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);
Expand All @@ -280,11 +277,9 @@ export default function run(config: Partial<Config>) {
}
}
} 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()}`,
Expand Down
26 changes: 10 additions & 16 deletions packages/node-renderer/src/worker/handleRenderRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -84,18 +84,14 @@ async function handleNewBundleProvided(
): Promise<ResponseResult> {
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);
Copy link
Contributor Author

@alexeyr-ci alexeyr-ci Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/shakacode/react_on_rails_pro/pull/517/files#r1956042299.

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));
}
Expand Down Expand Up @@ -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);
}
Expand Down
29 changes: 12 additions & 17 deletions packages/node-renderer/tests/handleRenderRequest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 => ({
Expand Down Expand Up @@ -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';
Copy link
Contributor Author

@alexeyr-ci alexeyr-ci Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proper-lockfile already uses mtime. But this test fails...


expect.assertions(2);
touch.sync(lockfilePathForTest(), { time: '1979-07-01T19:10:00.000Z' });
await createUploadedBundleForTest();
Expand All @@ -103,16 +97,17 @@ describe(testName, () => {
expect.assertions(2);
await createUploadedBundleForTest();

const lockfileOptions = { pollPeriod: 100, stale: 10000 };
lockfile.lockSync(lockfilePathForTest(), lockfileOptions);
const release = lockSync(
Copy link
Contributor Author

@alexeyr-ci alexeyr-ci Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was probably lockSync for simplicity, here we have a normal Promise-based API and can use lock instead. But it should wait until the other tests pass.

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',
Expand Down
38 changes: 26 additions & 12 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -7216,6 +7225,11 @@ [email protected]:
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"
Expand Down