From 96971aa4c48dbdfb24ed32ec3d14b311fc50a15d Mon Sep 17 00:00:00 2001 From: Kristóf Marussy Date: Mon, 30 May 2022 00:17:37 +0200 Subject: refactor(main): improve config file handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify config file watcher by debouncing events instead of throttling. Make sure to flush config changes on application exit. Signed-off-by: Kristóf Marussy --- .../src/infrastructure/config/ConfigRepository.ts | 2 +- .../src/infrastructure/config/impl/ConfigFile.ts | 23 ++++-- .../config/impl/__tests__/ConfigFile.integ.test.ts | 94 ++++++++++------------ 3 files changed, 60 insertions(+), 59 deletions(-) (limited to 'packages/main/src/infrastructure/config') diff --git a/packages/main/src/infrastructure/config/ConfigRepository.ts b/packages/main/src/infrastructure/config/ConfigRepository.ts index 7d5e12b..182f446 100644 --- a/packages/main/src/infrastructure/config/ConfigRepository.ts +++ b/packages/main/src/infrastructure/config/ConfigRepository.ts @@ -29,5 +29,5 @@ export default interface ConfigPersistence { writeConfig(contents: string): Promise; - watchConfig(callback: () => Promise, throttleMs: number): Disposer; + watchConfig(callback: () => Promise): Disposer; } diff --git a/packages/main/src/infrastructure/config/impl/ConfigFile.ts b/packages/main/src/infrastructure/config/impl/ConfigFile.ts index 4f0d4f0..8f0cc3f 100644 --- a/packages/main/src/infrastructure/config/impl/ConfigFile.ts +++ b/packages/main/src/infrastructure/config/impl/ConfigFile.ts @@ -22,7 +22,7 @@ import { watch } from 'node:fs'; import { readFile, stat, writeFile } from 'node:fs/promises'; import path from 'node:path'; -import { throttle } from 'lodash-es'; +import { debounce } from 'lodash-es'; import type Disposer from '../../../utils/Disposer.js'; import getLogger from '../../../utils/getLogger.js'; @@ -33,6 +33,7 @@ import type { ReadConfigResult } from '../ConfigRepository.js'; const log = getLogger('ConfigFile'); export const CONFIG_FILE_NAME = 'settings.json'; +export const DEFAULT_CONFIG_CHANGE_DEBOUNCE_MS = 10; export default class ConfigFile implements ConfigRepository { private readonly configFilePath: string; @@ -44,6 +45,7 @@ export default class ConfigFile implements ConfigRepository { constructor( private readonly userDataDir: string, private readonly configFileName = CONFIG_FILE_NAME, + private readonly debounceTime = DEFAULT_CONFIG_CHANGE_DEBOUNCE_MS, ) { this.configFilePath = path.join(userDataDir, configFileName); } @@ -59,7 +61,7 @@ export default class ConfigFile implements ConfigRepository { } throw error; } - log.info('Read config file', this.configFilePath); + log.debug('Read config file', this.configFilePath); return { found: true, contents, @@ -82,10 +84,10 @@ export default class ConfigFile implements ConfigRepository { log.debug('Wrote config file', this.configFilePath); } - watchConfig(callback: () => Promise, throttleMs: number): Disposer { + watchConfig(callback: () => Promise): Disposer { log.debug('Installing watcher for', this.userDataDir); - const configChanged = throttle(async () => { + const configChanged = debounce(async () => { let mtime: Date; try { const stats = await stat(this.configFilePath); @@ -116,9 +118,13 @@ export default class ConfigFile implements ConfigRepository { 'which is newer than last written', this.timeLastWritten, ); - await callback(); + try { + await callback(); + } catch (error) { + log.error('Callback error while listening for config changes', error); + } } - }, throttleMs); + }, this.debounceTime); const watcher = watch( this.userDataDir, @@ -127,8 +133,12 @@ export default class ConfigFile implements ConfigRepository { recursive: false, }, (_eventType, filename) => { + // We handle both `rename` and `change` events for maximum portability. + // This may result in multiple calls to `configChanged` for a single config change, + // so we debounce it with a short (imperceptible) delay. if (filename === this.configFileName || filename === null) { configChanged()?.catch((err) => { + // This should never happen, because `configChanged` handles all exceptions. log.error( 'Unhandled error while listening for config changes', err, @@ -141,6 +151,7 @@ export default class ConfigFile implements ConfigRepository { return () => { log.trace('Removing watcher for', this.configFilePath); watcher.close(); + configChanged.cancel(); }; } } diff --git a/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts b/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts index b61e85a..dd4aaaa 100644 --- a/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts +++ b/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts @@ -26,6 +26,7 @@ import { rename, rm, stat, + utimes, writeFile, } from 'node:fs/promises'; import { tmpdir } from 'node:os'; @@ -37,7 +38,7 @@ import { mocked } from 'jest-mock'; import Disposer from '../../../../utils/Disposer.js'; import ConfigFile, { CONFIG_FILE_NAME } from '../ConfigFile.js'; -const THROTTLE_MS = 1000; +const CONFIG_CHANGE_DEBOUNCE_MS = 10; let filesystemDelay = 100; let realSetTimeout: typeof setTimeout; @@ -45,35 +46,29 @@ let userDataDir: string | undefined; let configFilePath: string; let repository: ConfigFile; +async function realDelay(ms: number): Promise { + return new Promise((resolve) => { + realSetTimeout(() => resolve(), ms); + }); +} + /** * Wait for a short (real-time) delay to let node be notified of file changes. * - * Will also run all pending microtasks to completion. - * * Use the `SOPHIE_INTEG_TEST_DELAY` environmental variable to customize the delay * (e.g., quicker test execution on faster computers or longer wait time in CI). * The default delay is 100 ms, but as little as 10 ms might be enough, depending on your system. * - * @returns A promise that resolves in a short while. + * @param ms The time that should elapse after noticing the filesystem change. + * @returns A promise that resolves in a `filesystemDelay` ms. */ -function catchUpWithFilesystem(): Promise { - return new Promise((resolve, reject) => { - realSetTimeout(() => { - try { - jest.runAllTicks(); - } catch (error) { - reject(error); - return; - } - resolve(); - }, filesystemDelay); - }); -} - -async function catchUpWithFilesystemAndTimers(): Promise { - await catchUpWithFilesystem(); - jest.runAllTimers(); - await catchUpWithFilesystem(); +async function catchUpWithFilesystem( + ms = CONFIG_CHANGE_DEBOUNCE_MS, +): Promise { + await realDelay(filesystemDelay); + jest.advanceTimersByTime(ms); + // Some extra real time is needed after advancing the timer to make sure the callback runs. + await realDelay(Math.max(filesystemDelay / 10, 1)); } beforeAll(() => { @@ -91,7 +86,11 @@ beforeEach(async () => { try { userDataDir = await mkdtemp(path.join(tmpdir(), 'sophie-configFile-')); configFilePath = path.join(userDataDir, CONFIG_FILE_NAME); - repository = new ConfigFile(userDataDir); + repository = new ConfigFile( + userDataDir, + CONFIG_FILE_NAME, + CONFIG_CHANGE_DEBOUNCE_MS, + ); } catch (error) { userDataDir = undefined; throw error; @@ -180,7 +179,7 @@ describe('watchConfig', () => { describe('when the config file does not exist', () => { beforeEach(() => { - watcher = repository.watchConfig(callback, THROTTLE_MS); + watcher = repository.watchConfig(callback); }); test('notifies when the config file is created externally', async () => { @@ -191,7 +190,7 @@ describe('watchConfig', () => { test('does not notify when the config file is created by the repository', async () => { await repository.writeConfig('Hello World!'); - await catchUpWithFilesystemAndTimers(); + await catchUpWithFilesystem(); expect(callback).not.toHaveBeenCalled(); }); }); @@ -199,7 +198,7 @@ describe('watchConfig', () => { describe('when the config file already exists', () => { beforeEach(async () => { await writeFile(configFilePath, 'Hello World!', 'utf8'); - watcher = repository.watchConfig(callback, THROTTLE_MS); + watcher = repository.watchConfig(callback); }); test('notifies when the config file is updated externally', async () => { @@ -210,35 +209,23 @@ describe('watchConfig', () => { test('does not notify when the config file is created by the repository', async () => { await repository.writeConfig('Hi Mars!'); - await catchUpWithFilesystemAndTimers(); + await catchUpWithFilesystem(); expect(callback).not.toHaveBeenCalled(); }); - test('throttles notifications of external changes to the config file', async () => { + test('debounces changes to the config file', async () => { await writeFile(configFilePath, 'Hi Mars!', 'utf8'); - await catchUpWithFilesystem(); - expect(callback).toHaveBeenCalledTimes(1); - mocked(callback).mockClear(); - - jest.advanceTimersByTime(100); - await writeFile(configFilePath, 'Howdy Venus!', 'utf8'); - await catchUpWithFilesystem(); - - jest.advanceTimersByTime(100); - await catchUpWithFilesystem(); + await catchUpWithFilesystem(5); expect(callback).not.toHaveBeenCalled(); - jest.advanceTimersByTime(THROTTLE_MS); + await writeFile(configFilePath, 'Hi Mars!', 'utf8'); await catchUpWithFilesystem(); expect(callback).toHaveBeenCalledTimes(1); }); test('handles the config file being deleted', async () => { - await writeFile(configFilePath, 'Hi Mars!', 'utf8'); - // Clear the mock if we inadverently set off `callback`. - mocked(callback).mockClear(); await rm(configFilePath); - await catchUpWithFilesystemAndTimers(); + await catchUpWithFilesystem(); expect(callback).not.toHaveBeenCalled(); await writeFile(configFilePath, 'Hello World!', 'utf8'); @@ -249,15 +236,12 @@ describe('watchConfig', () => { test('handles the config file being renamed', async () => { const renamedPath = `${configFilePath}.renamed`; - await writeFile(configFilePath, 'Hi Mars!', 'utf8'); - // Clear the mock if we inadverently set off `callback`. - mocked(callback).mockClear(); await rename(configFilePath, renamedPath); - await catchUpWithFilesystemAndTimers(); + await catchUpWithFilesystem(); expect(callback).not.toHaveBeenCalled(); await writeFile(renamedPath, 'Hello World!', 'utf8'); - await catchUpWithFilesystemAndTimers(); + await catchUpWithFilesystem(); expect(callback).not.toHaveBeenCalled(); await writeFile(configFilePath, 'Hello World!', 'utf8'); @@ -266,21 +250,27 @@ describe('watchConfig', () => { }); test('handles other filesystem errors', async () => { - await writeFile(configFilePath, 'Hi Mars!', 'utf8'); - // Clear the mock if we inadverently set off `callback`. - mocked(callback).mockClear(); const { mode } = await stat(userDataDir!); + await writeFile(configFilePath, 'Hi Mars!', 'utf8'); // Remove permission to force a filesystem error. // eslint-disable-next-line no-bitwise -- Compute reduced permissions. await chmod(userDataDir!, mode & 0o666); try { - await catchUpWithFilesystemAndTimers(); + await catchUpWithFilesystem(); expect(callback).not.toHaveBeenCalled(); } finally { await chmod(userDataDir!, mode); } }); + test('does not notify when the modification date is prior to the last write', async () => { + await repository.writeConfig('Hello World!'); + const date = new Date(Date.now() - 3_600_000); + await utimes(configFilePath, date, date); + await catchUpWithFilesystem(); + expect(callback).not.toHaveBeenCalled(); + }); + test('handles callback errors', async () => { mocked(callback).mockRejectedValue(new Error('Test error')); await writeFile(configFilePath, 'Hi Mars!', 'utf8'); -- cgit v1.2.3-70-g09d2