Skip to content

Commit c808823

Browse files
alexeyralexeyr
committed
WIP Switch from lockfile to proper-lockfile
1 parent 73ba780 commit c808823

File tree

6 files changed

+94
-114
lines changed

6 files changed

+94
-114
lines changed

‎package.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
"@fastify/multipart": "^8.3.0 || ^9.0.1",
4545
"fastify": "^4.28.1 || ^5.1.0",
4646
"fs-extra": "^11.2.0",
47-
"lockfile": "^1.0.4"
47+
"proper-lockfile": "^4.1.2"
4848
},
4949
"devDependencies": {
5050
"@babel/core": "^7.12.3",
@@ -56,7 +56,7 @@
5656
"@tsconfig/node14": "^14.1.2",
5757
"@types/fs-extra": "^11.0.4",
5858
"@types/jest": "^29.5.12",
59-
"@types/lockfile": "^1.0.4",
59+
"@types/proper-lockfile": "^4.1.4",
6060
"@types/touch": "^3.1.5",
6161
"babel-jest": "^29.7.0",
6262
"concurrently": "^9.1.0",

‎packages/node-renderer/src/shared/locks.ts‎

Lines changed: 36 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,91 +1,73 @@
1-
import lockfile, { Options } from 'lockfile';
2-
import { promisify } from 'util';
1+
import { lock as lockLib, LockOptions } from 'proper-lockfile';
32

43
import debug from './debug';
54
import log from './log';
65
import { delay, workerIdLabel } from './utils';
76

8-
const lockfileLockAsync = promisify<string, Options>(lockfile.lock);
9-
const lockfileUnlockAsync = promisify(lockfile.unlock);
10-
117
const TEST_LOCKFILE_THREADING = false;
128

13-
// See definitions here: https://github.com/npm/lockfile/blob/master/README.md#options
14-
/*
15-
* A number of milliseconds to wait for locks to expire before giving up. Only used by
16-
* lockFile.lock. Poll for opts.wait ms. If the lock is not cleared by the time the wait expires,
17-
* then it returns with the original error.
18-
*/
19-
const LOCKFILE_WAIT = 3000;
20-
21-
/*
22-
* When using opts.wait, this is the period in ms in which it polls to check if the lock has
23-
* expired. Defaults to 100.
24-
*/
25-
const LOCKFILE_POLL_PERIOD = 300; // defaults to 100
26-
9+
// See definitions here: https://www.npmjs.com/package/proper-lockfile?activeTab=readme#lockfile-options
10+
// and https://www.npmjs.com/package/retry#retryoperationoptions for retries
2711
/*
2812
* A number of milliseconds before locks are considered to have expired.
2913
*/
3014
const LOCKFILE_STALE = 20000;
3115

3216
/*
33-
* Used by lock and lockSync. Retry n number of times before giving up.
17+
* The number of retries.
3418
*/
3519
const LOCKFILE_RETRIES = 45;
3620

3721
/*
38-
* Used by lock. Wait n milliseconds before retrying.
22+
* The number of milliseconds before starting the first retry.
3923
*/
40-
const LOCKFILE_RETRY_WAIT = 300;
24+
const LOCKFILE_RETRY_MIN_TIMEOUT = 300;
4125

42-
const lockfileOptions = {
43-
wait: LOCKFILE_WAIT,
44-
retryWait: LOCKFILE_RETRY_WAIT,
45-
retries: LOCKFILE_RETRIES,
26+
const lockfileOptions: LockOptions = {
27+
// so the argument doesn't have to be an existing file
28+
realpath: false,
29+
retries: {
30+
retries: LOCKFILE_RETRIES,
31+
minTimeout: LOCKFILE_RETRY_MIN_TIMEOUT,
32+
maxTimeout: 100 * LOCKFILE_RETRY_MIN_TIMEOUT,
33+
randomize: true,
34+
},
4635
stale: LOCKFILE_STALE,
47-
pollPeriod: LOCKFILE_POLL_PERIOD,
4836
};
4937

50-
export async function unlock(lockfileName: string) {
51-
debug('Worker %s: About to unlock %s', workerIdLabel(), lockfileName);
52-
log.info('Worker %s: About to unlock %s', workerIdLabel(), lockfileName);
53-
54-
await lockfileUnlockAsync(lockfileName);
55-
}
56-
57-
type LockResult = {
58-
lockfileName: string;
59-
} & (
60-
| {
61-
wasLockAcquired: true;
62-
errorMessage: null;
63-
}
64-
| {
65-
wasLockAcquired: false;
66-
errorMessage: Error;
67-
}
68-
);
38+
type LockResult = | {
39+
wasLockAcquired: true;
40+
release: () => Promise<void>;
41+
} | {
42+
wasLockAcquired: false;
43+
error: Error;
44+
};
6945

7046
export async function lock(filename: string): Promise<LockResult> {
71-
const lockfileName = `${filename}.lock`;
7247
const workerId = workerIdLabel();
7348

7449
try {
75-
debug('Worker %s: About to request lock %s', workerId, lockfileName);
76-
log.info('Worker %s: About to request lock %s', workerId, lockfileName);
77-
await lockfileLockAsync(lockfileName, lockfileOptions);
50+
debug('Worker %s: About to request lock %s', workerId, filename);
51+
log.info('Worker %s: About to request lock %s', workerId, filename);
52+
const releaseLib = await lockLib(filename, lockfileOptions);
7853

7954
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- the const may be changed to test threading
8055
if (TEST_LOCKFILE_THREADING) {
8156
debug('Worker %i: handleNewBundleProvided sleeping 5s', workerId);
8257
await delay(5000);
8358
debug('Worker %i: handleNewBundleProvided done sleeping 5s', workerId);
8459
}
85-
debug('After acquired lock in pid', lockfileName);
60+
debug('After acquired lock in pid', filename);
61+
return {
62+
wasLockAcquired: true,
63+
release: () => {
64+
debug('Worker %s: About to unlock %s', workerIdLabel(), filename);
65+
log.info('Worker %s: About to unlock %s', workerIdLabel(), filename);
66+
return releaseLib();
67+
},
68+
};
8669
} catch (error) {
87-
log.info('Worker %s: Failed to acquire lock %s, error %s', workerId, lockfileName, error);
88-
return { lockfileName, wasLockAcquired: false, errorMessage: error as Error };
70+
log.info('Worker %s: Failed to acquire lock %s, error %s', workerId, filename, error);
71+
return { wasLockAcquired: false, error: error as Error };
8972
}
90-
return { lockfileName, wasLockAcquired: true, errorMessage: null };
9173
}

‎packages/node-renderer/src/worker.ts‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
Asset,
2727
} from './shared/utils';
2828
import * as errorReporter from './shared/errorReporter';
29-
import { lock, unlock } from './shared/locks';
29+
import { lock } from './shared/locks';
3030
import { startSsrRequestOptions, trace } from './shared/tracing';
3131

3232
// Uncomment the below for testing timeouts:
@@ -240,20 +240,17 @@ export default function run(config: Partial<Config>) {
240240
if (!(await requestPrechecks(req, res))) {
241241
return;
242242
}
243-
let lockAcquired = false;
244-
let lockfileName: string | undefined;
245243
const assets: Asset[] = Object.values(req.body).filter(isAsset);
246244
const assetsDescription = JSON.stringify(assets.map((asset) => asset.filename));
247245
const taskDescription = `Uploading files ${assetsDescription} to ${bundlePath}`;
246+
const lockfileName = 'transferring-assets';
247+
// lock already catches errors internally, so it's safe to call without try/catch
248+
const lockResult = await lock(lockfileName);
248249
try {
249-
const { lockfileName: name, wasLockAcquired, errorMessage } = await lock('transferring-assets');
250-
lockfileName = name;
251-
lockAcquired = wasLockAcquired;
252-
253-
if (!wasLockAcquired) {
250+
if (!lockResult.wasLockAcquired) {
254251
const msg = formatExceptionMessage(
255252
taskDescription,
256-
errorMessage,
253+
lockResult.error,
257254
`Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`,
258255
);
259256
await setResponse(errorResponseResult(msg), res);
@@ -280,11 +277,9 @@ export default function run(config: Partial<Config>) {
280277
}
281278
}
282279
} finally {
283-
if (lockAcquired) {
280+
if (lockResult.wasLockAcquired) {
284281
try {
285-
if (lockfileName) {
286-
await unlock(lockfileName);
287-
}
282+
await lockResult.release();
288283
} catch (error) {
289284
log.warn({
290285
msg: `Error unlocking ${lockfileName} from worker ${workerIdLabel()}`,

‎packages/node-renderer/src/worker/handleRenderRequest.ts‎

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
import cluster from 'cluster';
99
import path from 'path';
10-
import { lock, unlock } from '../shared/locks';
10+
import { lock } from '../shared/locks';
1111
import fileExistsAsync from '../shared/fileExistsAsync';
1212
import log from '../shared/log';
1313
import {
@@ -84,18 +84,14 @@ async function handleNewBundleProvided(
8484
): Promise<ResponseResult> {
8585
log.info('Worker received new bundle: %s', bundleFilePathPerTimestamp);
8686

87-
let lockAcquired = false;
88-
let lockfileName: string | undefined;
87+
// lock already catches errors internally, so it's safe to call without try/catch
88+
const lockResult = await lock(bundleFilePathPerTimestamp);
8989
try {
90-
const { lockfileName: name, wasLockAcquired, errorMessage } = await lock(bundleFilePathPerTimestamp);
91-
lockfileName = name;
92-
lockAcquired = wasLockAcquired;
93-
94-
if (!wasLockAcquired) {
90+
if (!lockResult.wasLockAcquired) {
9591
const msg = formatExceptionMessage(
9692
renderingRequest,
97-
errorMessage,
98-
`Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`,
93+
lockResult.error,
94+
`Failed to acquire lock ${bundleFilePathPerTimestamp}. Worker: ${workerIdLabel()}.`,
9995
);
10096
return Promise.resolve(errorResponseResult(msg));
10197
}
@@ -143,17 +139,15 @@ to ${bundleFilePathPerTimestamp})`,
143139
return Promise.resolve(errorResponseResult(msg));
144140
}
145141
} finally {
146-
if (lockAcquired) {
147-
log.info('About to unlock %s from worker %i', lockfileName, workerIdLabel());
142+
if (lockResult.wasLockAcquired) {
143+
log.info('About to unlock %s from worker %i', bundleFilePathPerTimestamp, workerIdLabel());
148144
try {
149-
if (lockfileName) {
150-
await unlock(lockfileName);
151-
}
145+
await lockResult.release();
152146
} catch (error) {
153147
const msg = formatExceptionMessage(
154148
renderingRequest,
155149
error,
156-
`Error unlocking ${lockfileName} from worker ${workerIdLabel()}.`,
150+
`Error unlocking ${bundleFilePathPerTimestamp} from worker ${workerIdLabel()}.`,
157151
);
158152
log.warn(msg);
159153
}

‎packages/node-renderer/tests/handleRenderRequest.test.ts‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
import path from 'path';
22
import touch from 'touch';
3-
import lockfile from 'lockfile';
3+
import { lockSync } from 'proper-lockfile';
44
import {
5-
createVmBundle,
6-
uploadedBundlePath,
7-
createUploadedBundle,
8-
resetForTest,
95
BUNDLE_TIMESTAMP,
6+
createUploadedBundle,
7+
createVmBundle,
108
lockfilePath,
9+
resetForTest,
10+
uploadedBundlePath,
1111
} from './helper';
1212
import { getVmBundleFilePath } from '../src/worker/vm';
1313
import handleRenderRequest from '../src/worker/handleRenderRequest';
14-
import { delay, Asset } from '../src/shared/utils';
14+
import { Asset, delay } from '../src/shared/utils';
1515

1616
const testName = 'handleRenderRequest';
1717
const uploadedBundleForTest = (): Asset => ({
@@ -79,12 +79,6 @@ describe(testName, () => {
7979
});
8080

8181
test('If lockfile exists, and is stale', async () => {
82-
// We're using a lockfile with an artificially old date,
83-
// so make it use that instead of ctime.
84-
// Probably you should never do this in production!
85-
// @ts-expect-error Not allowed by the types
86-
lockfile.filetime = 'mtime';
87-
8882
expect.assertions(2);
8983
touch.sync(lockfilePathForTest(), { time: '1979-07-01T19:10:00.000Z' });
9084
await createUploadedBundleForTest();
@@ -103,16 +97,17 @@ describe(testName, () => {
10397
expect.assertions(2);
10498
await createUploadedBundleForTest();
10599

106-
const lockfileOptions = { pollPeriod: 100, stale: 10000 };
107-
lockfile.lockSync(lockfilePathForTest(), lockfileOptions);
100+
const release = lockSync(
101+
lockfilePathForTest(),
102+
{ stale: 10000, realpath: false },
103+
);
108104

109105
await delay(5);
110106
console.log('TEST building VM from sleep');
111107
await createVmBundleForTest();
112108
console.log('TEST DONE building VM from sleep');
113-
lockfile.unlock(lockfilePathForTest(), (err) => {
114-
console.log('TEST unlocked lockfile', err);
115-
});
109+
release();
110+
console.log('TEST unlocked lockfile');
116111

117112
const result = await handleRenderRequest({
118113
renderingRequest: 'ReactOnRails.dummy',

‎yarn.lock‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1789,11 +1789,6 @@
17891789
dependencies:
17901790
"@types/node" "*"
17911791

1792-
"@types/lockfile@^1.0.4":
1793-
version "1.0.4"
1794-
resolved "https://registry.yarnpkg.com/@types/lockfile/-/lockfile-1.0.4.tgz#9d6a6d1b6dbd4853cecc7f334bc53ea0ff363b8e"
1795-
integrity sha512-Q8oFIHJHr+htLrTXN2FuZfg+WXVHQRwU/hC2GpUu+Q8e3FUM9EDkS2pE3R2AO1ZGu56f479ybdMCNF1DAu8cAQ==
1796-
17971792
"@types/mime@^1":
17981793
version "1.3.5"
17991794
resolved "https://registry.yarnpkg.com/@types/mime/-/mime-1.3.5.tgz#1ef302e01cf7d2b5a0fa526790c9123bf1d06690"
@@ -1811,6 +1806,13 @@
18111806
resolved "https://registry.yarnpkg.com/@types/parse-json/-/parse-json-4.0.2.tgz#5950e50960793055845e956c427fc2b0d70c5239"
18121807
integrity sha512-dISoDXWWQwUquiKsyZ4Ng+HX2KsPL7LyHKHQwgGFEA3IaKac4Obd+h2a/a6waisAoepJlBcx9paWqjA8/HVjCw==
18131808

1809+
"@types/proper-lockfile@^4.1.4":
1810+
version "4.1.4"
1811+
resolved "https://registry.yarnpkg.com/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz#cd9fab92bdb04730c1ada542c356f03620f84008"
1812+
integrity sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==
1813+
dependencies:
1814+
"@types/retry" "*"
1815+
18141816
"@types/qs@*":
18151817
version "6.9.15"
18161818
resolved "https://registry.yarnpkg.com/@types/qs/-/qs-6.9.15.tgz#adde8a060ec9c305a82de1babc1056e73bd64dce"
@@ -1821,6 +1823,11 @@
18211823
resolved "https://registry.yarnpkg.com/@types/range-parser/-/range-parser-1.2.7.tgz#50ae4353eaaddc04044279812f52c8c65857dbcb"
18221824
integrity sha512-hKormJbkJqzQGhziax5PItDUTMAM9uE2XXQmM37dyd4hVM+5aVl7oVxMVUiVQn2oCQFN/LKCZdvSM0pFRqbSmQ==
18231825

1826+
"@types/retry@*":
1827+
version "0.12.5"
1828+
resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.5.tgz#f090ff4bd8d2e5b940ff270ab39fd5ca1834a07e"
1829+
integrity sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw==
1830+
18241831
"@types/semver@^7.3.12":
18251832
version "7.5.8"
18261833
resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.8.tgz#8268a8c57a3e4abd25c165ecd36237db7948a55e"
@@ -5831,13 +5838,6 @@ locate-path@^6.0.0:
58315838
dependencies:
58325839
p-locate "^5.0.0"
58335840

5834-
lockfile@^1.0.4:
5835-
version "1.0.4"
5836-
resolved "https://registry.yarnpkg.com/lockfile/-/lockfile-1.0.4.tgz#07f819d25ae48f87e538e6578b6964a4981a5609"
5837-
integrity sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA==
5838-
dependencies:
5839-
signal-exit "^3.0.2"
5840-
58415841
lodash.capitalize@^4.2.1:
58425842
version "4.2.1"
58435843
resolved "https://registry.yarnpkg.com/lodash.capitalize/-/lodash.capitalize-4.2.1.tgz#f826c9b4e2a8511d84e3aca29db05e1a4f3b72a9"
@@ -6789,6 +6789,15 @@ prop-types@^15.8.1:
67896789
object-assign "^4.1.1"
67906790
react-is "^16.13.1"
67916791

6792+
proper-lockfile@^4.1.2:
6793+
version "4.1.2"
6794+
resolved "https://registry.yarnpkg.com/proper-lockfile/-/proper-lockfile-4.1.2.tgz#c8b9de2af6b2f1601067f98e01ac66baa223141f"
6795+
integrity sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA==
6796+
dependencies:
6797+
graceful-fs "^4.2.4"
6798+
retry "^0.12.0"
6799+
signal-exit "^3.0.2"
6800+
67926801
proto-list@~1.2.1:
67936802
version "1.2.4"
67946803
resolved "https://registry.yarnpkg.com/proto-list/-/proto-list-1.2.4.tgz#212d5bfe1318306a420f6402b8e26ff39647a849"
@@ -7216,6 +7225,11 @@ [email protected]:
72167225
resolved "https://registry.yarnpkg.com/retry/-/retry-0.13.1.tgz#185b1587acf67919d63b357349e03537b2484658"
72177226
integrity sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg==
72187227

7228+
retry@^0.12.0:
7229+
version "0.12.0"
7230+
resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b"
7231+
integrity sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow==
7232+
72197233
reusify@^1.0.4:
72207234
version "1.0.4"
72217235
resolved "https://registry.yarnpkg.com/reusify/-/reusify-1.0.4.tgz#90da382b1e126efc02146e90845a88db12925d76"

0 commit comments

Comments
 (0)