diff options
author | Kristóf Marussy <kristof@marussy.com> | 2022-05-30 00:17:37 +0200 |
---|---|---|
committer | Kristóf Marussy <kristof@marussy.com> | 2022-05-30 00:28:47 +0200 |
commit | 96971aa4c48dbdfb24ed32ec3d14b311fc50a15d (patch) | |
tree | a7a233a03de6f9d8e445684405bb23e0946785ea /packages/main/src/infrastructure/config/impl | |
parent | test(main): ConfigFile integration test (diff) | |
download | sophie-96971aa4c48dbdfb24ed32ec3d14b311fc50a15d.tar.gz sophie-96971aa4c48dbdfb24ed32ec3d14b311fc50a15d.tar.zst sophie-96971aa4c48dbdfb24ed32ec3d14b311fc50a15d.zip |
refactor(main): improve config file handling
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 <kristof@marussy.com>
Diffstat (limited to 'packages/main/src/infrastructure/config/impl')
-rw-r--r-- | packages/main/src/infrastructure/config/impl/ConfigFile.ts | 23 | ||||
-rw-r--r-- | packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts | 94 |
2 files changed, 59 insertions, 58 deletions
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'; | |||
22 | import { readFile, stat, writeFile } from 'node:fs/promises'; | 22 | import { readFile, stat, writeFile } from 'node:fs/promises'; |
23 | import path from 'node:path'; | 23 | import path from 'node:path'; |
24 | 24 | ||
25 | import { throttle } from 'lodash-es'; | 25 | import { debounce } from 'lodash-es'; |
26 | 26 | ||
27 | import type Disposer from '../../../utils/Disposer.js'; | 27 | import type Disposer from '../../../utils/Disposer.js'; |
28 | import getLogger from '../../../utils/getLogger.js'; | 28 | import getLogger from '../../../utils/getLogger.js'; |
@@ -33,6 +33,7 @@ import type { ReadConfigResult } from '../ConfigRepository.js'; | |||
33 | const log = getLogger('ConfigFile'); | 33 | const log = getLogger('ConfigFile'); |
34 | 34 | ||
35 | export const CONFIG_FILE_NAME = 'settings.json'; | 35 | export const CONFIG_FILE_NAME = 'settings.json'; |
36 | export const DEFAULT_CONFIG_CHANGE_DEBOUNCE_MS = 10; | ||
36 | 37 | ||
37 | export default class ConfigFile implements ConfigRepository { | 38 | export default class ConfigFile implements ConfigRepository { |
38 | private readonly configFilePath: string; | 39 | private readonly configFilePath: string; |
@@ -44,6 +45,7 @@ export default class ConfigFile implements ConfigRepository { | |||
44 | constructor( | 45 | constructor( |
45 | private readonly userDataDir: string, | 46 | private readonly userDataDir: string, |
46 | private readonly configFileName = CONFIG_FILE_NAME, | 47 | private readonly configFileName = CONFIG_FILE_NAME, |
48 | private readonly debounceTime = DEFAULT_CONFIG_CHANGE_DEBOUNCE_MS, | ||
47 | ) { | 49 | ) { |
48 | this.configFilePath = path.join(userDataDir, configFileName); | 50 | this.configFilePath = path.join(userDataDir, configFileName); |
49 | } | 51 | } |
@@ -59,7 +61,7 @@ export default class ConfigFile implements ConfigRepository { | |||
59 | } | 61 | } |
60 | throw error; | 62 | throw error; |
61 | } | 63 | } |
62 | log.info('Read config file', this.configFilePath); | 64 | log.debug('Read config file', this.configFilePath); |
63 | return { | 65 | return { |
64 | found: true, | 66 | found: true, |
65 | contents, | 67 | contents, |
@@ -82,10 +84,10 @@ export default class ConfigFile implements ConfigRepository { | |||
82 | log.debug('Wrote config file', this.configFilePath); | 84 | log.debug('Wrote config file', this.configFilePath); |
83 | } | 85 | } |
84 | 86 | ||
85 | watchConfig(callback: () => Promise<void>, throttleMs: number): Disposer { | 87 | watchConfig(callback: () => Promise<void>): Disposer { |
86 | log.debug('Installing watcher for', this.userDataDir); | 88 | log.debug('Installing watcher for', this.userDataDir); |
87 | 89 | ||
88 | const configChanged = throttle(async () => { | 90 | const configChanged = debounce(async () => { |
89 | let mtime: Date; | 91 | let mtime: Date; |
90 | try { | 92 | try { |
91 | const stats = await stat(this.configFilePath); | 93 | const stats = await stat(this.configFilePath); |
@@ -116,9 +118,13 @@ export default class ConfigFile implements ConfigRepository { | |||
116 | 'which is newer than last written', | 118 | 'which is newer than last written', |
117 | this.timeLastWritten, | 119 | this.timeLastWritten, |
118 | ); | 120 | ); |
119 | await callback(); | 121 | try { |
122 | await callback(); | ||
123 | } catch (error) { | ||
124 | log.error('Callback error while listening for config changes', error); | ||
125 | } | ||
120 | } | 126 | } |
121 | }, throttleMs); | 127 | }, this.debounceTime); |
122 | 128 | ||
123 | const watcher = watch( | 129 | const watcher = watch( |
124 | this.userDataDir, | 130 | this.userDataDir, |
@@ -127,8 +133,12 @@ export default class ConfigFile implements ConfigRepository { | |||
127 | recursive: false, | 133 | recursive: false, |
128 | }, | 134 | }, |
129 | (_eventType, filename) => { | 135 | (_eventType, filename) => { |
136 | // We handle both `rename` and `change` events for maximum portability. | ||
137 | // This may result in multiple calls to `configChanged` for a single config change, | ||
138 | // so we debounce it with a short (imperceptible) delay. | ||
130 | if (filename === this.configFileName || filename === null) { | 139 | if (filename === this.configFileName || filename === null) { |
131 | configChanged()?.catch((err) => { | 140 | configChanged()?.catch((err) => { |
141 | // This should never happen, because `configChanged` handles all exceptions. | ||
132 | log.error( | 142 | log.error( |
133 | 'Unhandled error while listening for config changes', | 143 | 'Unhandled error while listening for config changes', |
134 | err, | 144 | err, |
@@ -141,6 +151,7 @@ export default class ConfigFile implements ConfigRepository { | |||
141 | return () => { | 151 | return () => { |
142 | log.trace('Removing watcher for', this.configFilePath); | 152 | log.trace('Removing watcher for', this.configFilePath); |
143 | watcher.close(); | 153 | watcher.close(); |
154 | configChanged.cancel(); | ||
144 | }; | 155 | }; |
145 | } | 156 | } |
146 | } | 157 | } |
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 { | |||
26 | rename, | 26 | rename, |
27 | rm, | 27 | rm, |
28 | stat, | 28 | stat, |
29 | utimes, | ||
29 | writeFile, | 30 | writeFile, |
30 | } from 'node:fs/promises'; | 31 | } from 'node:fs/promises'; |
31 | import { tmpdir } from 'node:os'; | 32 | import { tmpdir } from 'node:os'; |
@@ -37,7 +38,7 @@ import { mocked } from 'jest-mock'; | |||
37 | import Disposer from '../../../../utils/Disposer.js'; | 38 | import Disposer from '../../../../utils/Disposer.js'; |
38 | import ConfigFile, { CONFIG_FILE_NAME } from '../ConfigFile.js'; | 39 | import ConfigFile, { CONFIG_FILE_NAME } from '../ConfigFile.js'; |
39 | 40 | ||
40 | const THROTTLE_MS = 1000; | 41 | const CONFIG_CHANGE_DEBOUNCE_MS = 10; |
41 | 42 | ||
42 | let filesystemDelay = 100; | 43 | let filesystemDelay = 100; |
43 | let realSetTimeout: typeof setTimeout; | 44 | let realSetTimeout: typeof setTimeout; |
@@ -45,35 +46,29 @@ let userDataDir: string | undefined; | |||
45 | let configFilePath: string; | 46 | let configFilePath: string; |
46 | let repository: ConfigFile; | 47 | let repository: ConfigFile; |
47 | 48 | ||
49 | async function realDelay(ms: number): Promise<void> { | ||
50 | return new Promise<void>((resolve) => { | ||
51 | realSetTimeout(() => resolve(), ms); | ||
52 | }); | ||
53 | } | ||
54 | |||
48 | /** | 55 | /** |
49 | * Wait for a short (real-time) delay to let node be notified of file changes. | 56 | * Wait for a short (real-time) delay to let node be notified of file changes. |
50 | * | 57 | * |
51 | * Will also run all pending microtasks to completion. | ||
52 | * | ||
53 | * Use the `SOPHIE_INTEG_TEST_DELAY` environmental variable to customize the delay | 58 | * Use the `SOPHIE_INTEG_TEST_DELAY` environmental variable to customize the delay |
54 | * (e.g., quicker test execution on faster computers or longer wait time in CI). | 59 | * (e.g., quicker test execution on faster computers or longer wait time in CI). |
55 | * The default delay is 100 ms, but as little as 10 ms might be enough, depending on your system. | 60 | * The default delay is 100 ms, but as little as 10 ms might be enough, depending on your system. |
56 | * | 61 | * |
57 | * @returns A promise that resolves in a short while. | 62 | * @param ms The time that should elapse after noticing the filesystem change. |
63 | * @returns A promise that resolves in a `filesystemDelay` ms. | ||
58 | */ | 64 | */ |
59 | function catchUpWithFilesystem(): Promise<void> { | 65 | async function catchUpWithFilesystem( |
60 | return new Promise((resolve, reject) => { | 66 | ms = CONFIG_CHANGE_DEBOUNCE_MS, |
61 | realSetTimeout(() => { | 67 | ): Promise<void> { |
62 | try { | 68 | await realDelay(filesystemDelay); |
63 | jest.runAllTicks(); | 69 | jest.advanceTimersByTime(ms); |
64 | } catch (error) { | 70 | // Some extra real time is needed after advancing the timer to make sure the callback runs. |
65 | reject(error); | 71 | await realDelay(Math.max(filesystemDelay / 10, 1)); |
66 | return; | ||
67 | } | ||
68 | resolve(); | ||
69 | }, filesystemDelay); | ||
70 | }); | ||
71 | } | ||
72 | |||
73 | async function catchUpWithFilesystemAndTimers(): Promise<void> { | ||
74 | await catchUpWithFilesystem(); | ||
75 | jest.runAllTimers(); | ||
76 | await catchUpWithFilesystem(); | ||
77 | } | 72 | } |
78 | 73 | ||
79 | beforeAll(() => { | 74 | beforeAll(() => { |
@@ -91,7 +86,11 @@ beforeEach(async () => { | |||
91 | try { | 86 | try { |
92 | userDataDir = await mkdtemp(path.join(tmpdir(), 'sophie-configFile-')); | 87 | userDataDir = await mkdtemp(path.join(tmpdir(), 'sophie-configFile-')); |
93 | configFilePath = path.join(userDataDir, CONFIG_FILE_NAME); | 88 | configFilePath = path.join(userDataDir, CONFIG_FILE_NAME); |
94 | repository = new ConfigFile(userDataDir); | 89 | repository = new ConfigFile( |
90 | userDataDir, | ||
91 | CONFIG_FILE_NAME, | ||
92 | CONFIG_CHANGE_DEBOUNCE_MS, | ||
93 | ); | ||
95 | } catch (error) { | 94 | } catch (error) { |
96 | userDataDir = undefined; | 95 | userDataDir = undefined; |
97 | throw error; | 96 | throw error; |
@@ -180,7 +179,7 @@ describe('watchConfig', () => { | |||
180 | 179 | ||
181 | describe('when the config file does not exist', () => { | 180 | describe('when the config file does not exist', () => { |
182 | beforeEach(() => { | 181 | beforeEach(() => { |
183 | watcher = repository.watchConfig(callback, THROTTLE_MS); | 182 | watcher = repository.watchConfig(callback); |
184 | }); | 183 | }); |
185 | 184 | ||
186 | test('notifies when the config file is created externally', async () => { | 185 | test('notifies when the config file is created externally', async () => { |
@@ -191,7 +190,7 @@ describe('watchConfig', () => { | |||
191 | 190 | ||
192 | test('does not notify when the config file is created by the repository', async () => { | 191 | test('does not notify when the config file is created by the repository', async () => { |
193 | await repository.writeConfig('Hello World!'); | 192 | await repository.writeConfig('Hello World!'); |
194 | await catchUpWithFilesystemAndTimers(); | 193 | await catchUpWithFilesystem(); |
195 | expect(callback).not.toHaveBeenCalled(); | 194 | expect(callback).not.toHaveBeenCalled(); |
196 | }); | 195 | }); |
197 | }); | 196 | }); |
@@ -199,7 +198,7 @@ describe('watchConfig', () => { | |||
199 | describe('when the config file already exists', () => { | 198 | describe('when the config file already exists', () => { |
200 | beforeEach(async () => { | 199 | beforeEach(async () => { |
201 | await writeFile(configFilePath, 'Hello World!', 'utf8'); | 200 | await writeFile(configFilePath, 'Hello World!', 'utf8'); |
202 | watcher = repository.watchConfig(callback, THROTTLE_MS); | 201 | watcher = repository.watchConfig(callback); |
203 | }); | 202 | }); |
204 | 203 | ||
205 | test('notifies when the config file is updated externally', async () => { | 204 | test('notifies when the config file is updated externally', async () => { |
@@ -210,35 +209,23 @@ describe('watchConfig', () => { | |||
210 | 209 | ||
211 | test('does not notify when the config file is created by the repository', async () => { | 210 | test('does not notify when the config file is created by the repository', async () => { |
212 | await repository.writeConfig('Hi Mars!'); | 211 | await repository.writeConfig('Hi Mars!'); |
213 | await catchUpWithFilesystemAndTimers(); | 212 | await catchUpWithFilesystem(); |
214 | expect(callback).not.toHaveBeenCalled(); | 213 | expect(callback).not.toHaveBeenCalled(); |
215 | }); | 214 | }); |
216 | 215 | ||
217 | test('throttles notifications of external changes to the config file', async () => { | 216 | test('debounces changes to the config file', async () => { |
218 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); | 217 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); |
219 | await catchUpWithFilesystem(); | 218 | await catchUpWithFilesystem(5); |
220 | expect(callback).toHaveBeenCalledTimes(1); | ||
221 | mocked(callback).mockClear(); | ||
222 | |||
223 | jest.advanceTimersByTime(100); | ||
224 | await writeFile(configFilePath, 'Howdy Venus!', 'utf8'); | ||
225 | await catchUpWithFilesystem(); | ||
226 | |||
227 | jest.advanceTimersByTime(100); | ||
228 | await catchUpWithFilesystem(); | ||
229 | expect(callback).not.toHaveBeenCalled(); | 219 | expect(callback).not.toHaveBeenCalled(); |
230 | 220 | ||
231 | jest.advanceTimersByTime(THROTTLE_MS); | 221 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); |
232 | await catchUpWithFilesystem(); | 222 | await catchUpWithFilesystem(); |
233 | expect(callback).toHaveBeenCalledTimes(1); | 223 | expect(callback).toHaveBeenCalledTimes(1); |
234 | }); | 224 | }); |
235 | 225 | ||
236 | test('handles the config file being deleted', async () => { | 226 | test('handles the config file being deleted', async () => { |
237 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); | ||
238 | // Clear the mock if we inadverently set off `callback`. | ||
239 | mocked(callback).mockClear(); | ||
240 | await rm(configFilePath); | 227 | await rm(configFilePath); |
241 | await catchUpWithFilesystemAndTimers(); | 228 | await catchUpWithFilesystem(); |
242 | expect(callback).not.toHaveBeenCalled(); | 229 | expect(callback).not.toHaveBeenCalled(); |
243 | 230 | ||
244 | await writeFile(configFilePath, 'Hello World!', 'utf8'); | 231 | await writeFile(configFilePath, 'Hello World!', 'utf8'); |
@@ -249,15 +236,12 @@ describe('watchConfig', () => { | |||
249 | test('handles the config file being renamed', async () => { | 236 | test('handles the config file being renamed', async () => { |
250 | const renamedPath = `${configFilePath}.renamed`; | 237 | const renamedPath = `${configFilePath}.renamed`; |
251 | 238 | ||
252 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); | ||
253 | // Clear the mock if we inadverently set off `callback`. | ||
254 | mocked(callback).mockClear(); | ||
255 | await rename(configFilePath, renamedPath); | 239 | await rename(configFilePath, renamedPath); |
256 | await catchUpWithFilesystemAndTimers(); | 240 | await catchUpWithFilesystem(); |
257 | expect(callback).not.toHaveBeenCalled(); | 241 | expect(callback).not.toHaveBeenCalled(); |
258 | 242 | ||
259 | await writeFile(renamedPath, 'Hello World!', 'utf8'); | 243 | await writeFile(renamedPath, 'Hello World!', 'utf8'); |
260 | await catchUpWithFilesystemAndTimers(); | 244 | await catchUpWithFilesystem(); |
261 | expect(callback).not.toHaveBeenCalled(); | 245 | expect(callback).not.toHaveBeenCalled(); |
262 | 246 | ||
263 | await writeFile(configFilePath, 'Hello World!', 'utf8'); | 247 | await writeFile(configFilePath, 'Hello World!', 'utf8'); |
@@ -266,21 +250,27 @@ describe('watchConfig', () => { | |||
266 | }); | 250 | }); |
267 | 251 | ||
268 | test('handles other filesystem errors', async () => { | 252 | test('handles other filesystem errors', async () => { |
269 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); | ||
270 | // Clear the mock if we inadverently set off `callback`. | ||
271 | mocked(callback).mockClear(); | ||
272 | const { mode } = await stat(userDataDir!); | 253 | const { mode } = await stat(userDataDir!); |
254 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); | ||
273 | // Remove permission to force a filesystem error. | 255 | // Remove permission to force a filesystem error. |
274 | // eslint-disable-next-line no-bitwise -- Compute reduced permissions. | 256 | // eslint-disable-next-line no-bitwise -- Compute reduced permissions. |
275 | await chmod(userDataDir!, mode & 0o666); | 257 | await chmod(userDataDir!, mode & 0o666); |
276 | try { | 258 | try { |
277 | await catchUpWithFilesystemAndTimers(); | 259 | await catchUpWithFilesystem(); |
278 | expect(callback).not.toHaveBeenCalled(); | 260 | expect(callback).not.toHaveBeenCalled(); |
279 | } finally { | 261 | } finally { |
280 | await chmod(userDataDir!, mode); | 262 | await chmod(userDataDir!, mode); |
281 | } | 263 | } |
282 | }); | 264 | }); |
283 | 265 | ||
266 | test('does not notify when the modification date is prior to the last write', async () => { | ||
267 | await repository.writeConfig('Hello World!'); | ||
268 | const date = new Date(Date.now() - 3_600_000); | ||
269 | await utimes(configFilePath, date, date); | ||
270 | await catchUpWithFilesystem(); | ||
271 | expect(callback).not.toHaveBeenCalled(); | ||
272 | }); | ||
273 | |||
284 | test('handles callback errors', async () => { | 274 | test('handles callback errors', async () => { |
285 | mocked(callback).mockRejectedValue(new Error('Test error')); | 275 | mocked(callback).mockRejectedValue(new Error('Test error')); |
286 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); | 276 | await writeFile(configFilePath, 'Hi Mars!', 'utf8'); |