From 190a3566f845a0986f38125ff1d3e4057c95c1f8 Mon Sep 17 00:00:00 2001 From: Johan Nyman Date: Fri, 16 Feb 2024 15:31:07 +0100 Subject: [PATCH] chore: refactor & fixes, co-authored by @julusian --- .../lib/__tests__/json-write-file.spec.ts | 44 ++++++++++-- .../accessorHandlers/lib/json-write-file.ts | 67 +++++++++---------- 2 files changed, 68 insertions(+), 43 deletions(-) diff --git a/shared/packages/worker/src/worker/accessorHandlers/lib/__tests__/json-write-file.spec.ts b/shared/packages/worker/src/worker/accessorHandlers/lib/__tests__/json-write-file.spec.ts index 5dd37629..275380e3 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/lib/__tests__/json-write-file.spec.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/lib/__tests__/json-write-file.spec.ts @@ -1,11 +1,22 @@ import { getTmpPath, updateJSONFile, updateJSONFileBatch } from '../json-write-file' import { promises as fs } from 'fs' +const logWarning = jest.fn((message: string) => console.log('WARNING', message)) +const logError = jest.fn((message: any) => console.log('ERROR', message)) + const FILE_A = 'file_a.json' async function cleanup() { + logWarning.mockClear() + logError.mockClear() + await Promise.all([unlinkIfExists(FILE_A), unlinkIfExists(getLockPath(FILE_A)), unlinkIfExists(getTmpPath(FILE_A))]) } +const config = { + logError, + logWarning, +} + beforeEach(cleanup) afterEach(cleanup) @@ -15,7 +26,7 @@ test('updateJSONFile: single write', async () => { a: 1, } }) - await updateJSONFile(FILE_A, cbManipulate) + await updateJSONFile(FILE_A, cbManipulate, config) expect(cbManipulate).toBeCalledTimes(1) expect(await readIfExists(FILE_A)).toBe( @@ -23,6 +34,8 @@ test('updateJSONFile: single write', async () => { a: 1, }) ) + expect(logWarning).toBeCalledTimes(0) + expect(logError).toBeCalledTimes(0) }) test('updateJSONFile: 2 writes', async () => { @@ -32,15 +45,17 @@ test('updateJSONFile: 2 writes', async () => { return o }) - const p0 = updateJSONFile(FILE_A, cbManipulate) + const p0 = updateJSONFile(FILE_A, cbManipulate, config) await sleep(5) - const p1 = updateJSONFile(FILE_A, cbManipulate) + const p1 = updateJSONFile(FILE_A, cbManipulate, config) await Promise.all([p0, p1]) expect(cbManipulate).toBeCalledTimes(2) expect(await readIfExists(FILE_A)).toBe(JSON.stringify(['a', 'a'])) + expect(logWarning).toBeCalledTimes(0) + expect(logError).toBeCalledTimes(0) }) test('updateJSONFile: 10 writes', async () => { const cbManipulate = jest.fn((o) => { @@ -50,6 +65,8 @@ test('updateJSONFile: 10 writes', async () => { }) const config = { + logError, + logWarning, retryTimeout: 30, retryCount: 3, } @@ -77,6 +94,9 @@ test('updateJSONFile: 10 writes', async () => { // Wait for the lock functions to finish retrying: await sleep(config.retryTimeout * config.retryCount) + + expect(logWarning).toBeCalledTimes(0) + expect(logError).toBeCalledTimes(0) }) test('updateJSONFileBatch: single write', async () => { @@ -85,7 +105,7 @@ test('updateJSONFileBatch: single write', async () => { b: 1, } }) - await updateJSONFileBatch(FILE_A, cbManipulate) + await updateJSONFileBatch(FILE_A, cbManipulate, config) expect(cbManipulate).toBeCalledTimes(1) expect(await readIfExists(FILE_A)).toBe( @@ -93,6 +113,8 @@ test('updateJSONFileBatch: single write', async () => { b: 1, }) ) + expect(logWarning).toBeCalledTimes(0) + expect(logError).toBeCalledTimes(0) }) test('updateJSONFileBatch: 3 writes', async () => { @@ -105,16 +127,19 @@ test('updateJSONFileBatch: 3 writes', async () => { return o }) - const p0 = updateJSONFileBatch(FILE_A, cbManipulate) + const p0 = updateJSONFileBatch(FILE_A, cbManipulate, config) await sleep(5) - const p1 = updateJSONFileBatch(FILE_A, cbManipulate) - const p2 = updateJSONFileBatch(FILE_A, cbManipulate) + const p1 = updateJSONFileBatch(FILE_A, cbManipulate, config) + const p2 = updateJSONFileBatch(FILE_A, cbManipulate, config) await Promise.all([p0, p1, p2]) expect(cbManipulate).toBeCalledTimes(3) expect(await readIfExists(FILE_A)).toBe(JSON.stringify(['a', 'a', 'a'])) + + expect(logWarning).toBeCalledTimes(0) + expect(logError).toBeCalledTimes(0) }) test('updateJSONFileBatch: 20 writes', async () => { const cbManipulate = jest.fn((o) => { @@ -124,6 +149,8 @@ test('updateJSONFileBatch: 20 writes', async () => { }) const config = { + logWarning, + logError, retryTimeout: 30, retryCount: 3, } @@ -139,6 +166,9 @@ test('updateJSONFileBatch: 20 writes', async () => { expect(cbManipulate).toBeCalledTimes(20) expect(await readIfExists(FILE_A)).toBe(JSON.stringify(expectResult)) + + expect(logWarning).toBeCalledTimes(0) + expect(logError).toBeCalledTimes(0) }) async function readIfExists(filePath: string): Promise { diff --git a/shared/packages/worker/src/worker/accessorHandlers/lib/json-write-file.ts b/shared/packages/worker/src/worker/accessorHandlers/lib/json-write-file.ts index 6ef7b1c9..5a13fa05 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/lib/json-write-file.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/lib/json-write-file.ts @@ -13,29 +13,22 @@ export async function updateJSONFileBatch( ): Promise { // Add manipulator callback to queue: - const existingBatches = updateJSONFileBatches.get(filePath) - let batches: BatchOperation[] - if (existingBatches) { - batches = existingBatches - } else { - batches = [] - updateJSONFileBatches.set(filePath, batches) - } - - // Find a batch operation that is open to accept new callbacks: - const openBatch = batches.find((batch) => batch.open) + const openBatch = updateJSONFileBatches.get(filePath) if (!openBatch) { // Start a new batch: const newBatch: BatchOperation = { - open: true, callbacks: [cbManipulate], promise: updateJSONFile( filePath, (oldValue: T | undefined) => { - // At this point, we close the batch, so no more callbacks can be added: - newBatch.open = false + // At this point, no more callbacks can be added, so we close the batch: + + // Guard against this being called multiple times: + if (updateJSONFileBatches.get(filePath) === newBatch) { + updateJSONFileBatches.delete(filePath) + } // Execute all callbacks in the batch: let value = oldValue @@ -47,20 +40,9 @@ export async function updateJSONFileBatch( config ), } - batches.push(newBatch) + updateJSONFileBatches.set(filePath, newBatch) - let caughtError: any = undefined - try { - await newBatch.promise - } catch (e) { - caughtError = e - } - // After finished executing, remove the batch: - const i = batches.indexOf(newBatch) - if (i === -1) throw new Error('Internal Error: Batch not found') - batches.splice(i, 1) - - if (caughtError) throw caughtError + await newBatch.promise } else { // There is a batch open for new callbacks. Add the callback to the batch: openBatch.callbacks.push(cbManipulate) @@ -68,10 +50,8 @@ export async function updateJSONFileBatch( } } -const updateJSONFileBatches = new Map() +const updateJSONFileBatches = new Map() interface BatchOperation { - /** When true, new callbacks can be added */ - open: boolean /** Resolves when the batch operation has finished */ promise: Promise callbacks: ((oldValue: any | undefined) => any | undefined)[] @@ -80,28 +60,41 @@ interface BatchOperation { /** * Read a JSON file, created by updateJSONFile() */ -export async function readJSONFile(filePath: string): Promise< +export async function readJSONFile( + filePath: string, + logError?: (message: any) => void +): Promise< | { str: string value: any } | undefined > { - { + // eslint-disable-next-line no-console + logError = logError ?? console.error + + try { const str = await readIfExists(filePath) if (str !== undefined) { return { str, value: str ? JSON.parse(str) : undefined } } + } catch (e) { + // file data is corrupt, log and continue + logError(e) } // Second try; Check if there is a temporary file, to use instead? - { + try { const tmpPath = getTmpPath(filePath) const str = await readIfExists(tmpPath) if (str !== undefined) { return { str, value: str ? JSON.parse(str) : undefined } } + } catch (e) { + logError(e) + // file data is corrupt, return undefined then + return undefined } return undefined @@ -109,7 +102,7 @@ export async function readJSONFile(filePath: string): Promise< /** * A "safe" way to write JSON data to a file. Takes measures to avoid writing corrupt data to a file due to - * 1. Multiple process writing to the same file (uses a lock file) + * 1. Multiple processes writing to the same file (uses a lock file) * 2. Writing corrupt files due to process exit (write to temporary file and rename) */ export async function updateJSONFile( @@ -166,7 +159,7 @@ export async function updateJSONFile( // At this point, we have acquired the lock. try { // Read and write to the file: - const oldValue = await readJSONFile(filePath) + const oldValue = await readJSONFile(filePath, logError) const newValue = cbManipulate(oldValue?.value) const newValueStr = newValue !== undefined ? JSON.stringify(newValue) : '' @@ -179,11 +172,13 @@ export async function updateJSONFile( continue } + // Note: We can't unlink the file anywhere in here, or other calls to Lockfile can break + // by overwriting the file with an empty one. + // Write to a temporary file first, to avoid corrupting the file in case of a process exit: await fs.writeFile(tmpFilePath, newValueStr) // Rename file: - await rename(tmpFilePath, filePath) }