diff --git a/spec/filesystem-manager-spec.js b/spec/filesystem-manager-spec.js index 29e0ca435..380419f7f 100644 --- a/spec/filesystem-manager-spec.js +++ b/spec/filesystem-manager-spec.js @@ -93,14 +93,17 @@ describe('FileSystemManager', function () { const sub1 = watcher.onDidChange(() => {}) await watcher.getStartPromise() - expect(watcher.native).not.toBe(null) - expect(watcher.native.isRunning()).toBe(true) + const native = watcher.native + expect(native).not.toBe(null) + expect(native.isRunning()).toBe(true) sub0.dispose() - expect(watcher.native.isRunning()).toBe(true) + expect(watcher.native).toBe(native) + expect(native.isRunning()).toBe(true) sub1.dispose() - expect(watcher.native.isRunning()).toBe(false) + expect(watcher.native).toBeNull() + expect(native.isRunning()).toBe(false) }) it('reuses an existing native watcher and resolves getStartPromise immediately if attached to a running watcher', async function () { @@ -117,48 +120,73 @@ describe('FileSystemManager', function () { expect(watcher0.native).toBe(watcher1.native) }) - it('reuses an existing native watcher on a parent directory and filters events', async function () { - const rootDir = await temp.mkdir('atom-fsmanager-').then(fs.realpath) + it("reuses existing native watchers even while they're still starting", async function () { + const rootDir = await temp.mkdir('atom-fsmanager-') + const watcher0 = manager.getWatcher(rootDir) + watcher0.onDidChange(() => {}) + await watcher0.getAttachedPromise() + expect(watcher0.native.isRunning()).toBe(false) + + const watcher1 = manager.getWatcher(rootDir) + watcher1.onDidChange(() => {}) + await watcher1.getAttachedPromise() + + expect(watcher0.native).toBe(watcher1.native) + + await Promise.all([watcher0.getStartPromise(), watcher1.getStartPromise()]) + }) + + it("doesn't attach new watchers to a native watcher that's stopping", async function () { + const rootDir = await temp.mkdir('atom-fsmanager-') + + const watcher0 = manager.getWatcher(rootDir) + const sub = watcher0.onDidChange(() => {}) + await watcher0.getStartPromise() + const native0 = watcher0.native + + sub.dispose() + + const watcher1 = manager.getWatcher(rootDir) + watcher1.onDidChange(() => {}) + + expect(watcher1.native).not.toBe(native0) + }) + + it('reuses an existing native watcher on a parent directory and filters events', async function () { + const rootDir = await temp.mkdir('atom-fsmanager-0-').then(fs.realpath) const rootFile = path.join(rootDir, 'rootfile.txt') const subDir = path.join(rootDir, 'subdir') const subFile = path.join(subDir, 'subfile.txt') - await Promise.all([ - fs.mkdir(subDir).then( - fs.writeFile(subFile, 'subfile\n', {encoding: 'utf8'}) - ), - fs.writeFile(rootFile, 'rootfile\n', {encoding: 'utf8'}) - ]) + await fs.mkdir(subDir) + + // Keep the watchers alive with an undisposed subscription const rootWatcher = manager.getWatcher(rootDir) + rootWatcher.onDidChange(() => {}) const childWatcher = manager.getWatcher(subDir) - - expect(rootWatcher.native).toBe(childWatcher.native) - - const firstRootChange = waitForChanges(rootWatcher, subFile) - const firstChildChange = waitForChanges(childWatcher, subFile) + childWatcher.onDidChange(() => {}) await Promise.all([ rootWatcher.getStartPromise(), childWatcher.getStartPromise() ]) - await fs.appendFile(subFile, 'changes\n', {encoding: 'utf8'}) + expect(rootWatcher.native).toBe(childWatcher.native) + expect(rootWatcher.native.isRunning()).toBe(true) - const firstPayloads = await Promise.all([firstRootChange, firstChildChange]) + const firstChanges = Promise.all([ + waitForChanges(rootWatcher, subFile), + waitForChanges(childWatcher, subFile) + ]) - for (const events of firstPayloads) { - expect(events.length).toBe(1) - expect(events[0].oldPath).toBe(subFile) - } + await fs.writeFile(subFile, 'subfile\n', {encoding: 'utf8'}) + await firstChanges const nextRootEvent = waitForChanges(rootWatcher, rootFile) - await fs.appendFile(rootFile, 'changes\n', {encoding: 'utf8'}) + await fs.writeFile(rootFile, 'rootfile\n', {encoding: 'utf8'}) - const nextPayload = await nextRootEvent - - expect(nextPayload.length).toBe(1) - expect(nextPayload[0].oldPath).toBe(rootFile) + await nextRootEvent }) it('adopts existing child watchers and filters events appropriately to them', async function () { @@ -181,17 +209,17 @@ describe('FileSystemManager', function () { ) ]) - // Begin the child watchers + // Begin the child watchers and keep them alive const subWatcher0 = manager.getWatcher(subDir0) + subWatcher0.onDidChange(() => {}) const subWatcherChanges0 = waitForChanges(subWatcher0, subFile0) const subWatcher1 = manager.getWatcher(subDir1) + subWatcher1.onDidChange(() => {}) const subWatcherChanges1 = waitForChanges(subWatcher1, subFile1) await Promise.all( - [subWatcher0, subWatcher1].map(watcher => { - return watcher.getStartPromise() - }) + [subWatcher0, subWatcher1].map(watcher => watcher.getStartPromise()) ) expect(subWatcher0.native).not.toBe(subWatcher1.native) diff --git a/spec/native-watcher-registry-spec.js b/spec/native-watcher-registry-spec.js index d76d91547..206aefc76 100644 --- a/spec/native-watcher-registry-spec.js +++ b/spec/native-watcher-registry-spec.js @@ -41,8 +41,8 @@ class MockNative { this.attached = [] } - onDidStop (callback) { - return this.emitter.on('did-stop', callback) + onWillStop (callback) { + return this.emitter.on('will-stop', callback) } dispose () { @@ -51,7 +51,7 @@ class MockNative { stop () { this.stopped = true - this.emitter.emit('did-stop') + this.emitter.emit('will-stop') } } diff --git a/src/filesystem-manager.js b/src/filesystem-manager.js index 3096e237a..277c7cbfe 100644 --- a/src/filesystem-manager.js +++ b/src/filesystem-manager.js @@ -16,6 +16,14 @@ const ACTION_MAP = new Map([ [nsfw.actions.RENAMED, 'renamed'] ]) +// Private: Possible states of a {NativeWatcher}. +export const WATCHER_STATE = { + STOPPED: Symbol('stopped'), + STARTING: Symbol('starting'), + RUNNING: Symbol('running'), + STOPPING: Symbol('stopping') +} + // Private: Interface with and normalize events from a native OS filesystem watcher. class NativeWatcher { @@ -27,16 +35,17 @@ class NativeWatcher { this.emitter = new Emitter() this.watcher = null - this.running = false + this.state = WATCHER_STATE.STOPPED } // Private: Begin watching for filesystem events. // // Has no effect if the watcher has already been started. async start () { - if (this.running) { + if (this.state !== WATCHER_STATE.STOPPED) { return } + this.state = WATCHER_STATE.STARTING this.watcher = await nsfw( this.normalizedPath, @@ -49,13 +58,14 @@ class NativeWatcher { await this.watcher.start() - this.running = true + + this.state = WATCHER_STATE.RUNNING this.emitter.emit('did-start') } - // Private: Return true if the underlying watcher has been started. + // Private: Return true if the underlying watcher is actively listening for filesystem events. isRunning () { - return this.running + return this.state === WATCHER_STATE.RUNNING } // Private: Register a callback to be invoked when the filesystem watcher has been initialized. @@ -71,9 +81,7 @@ class NativeWatcher { // // Returns: A {Disposable} to revoke the subscription. onDidChange (callback) { - if (!this.isRunning()) { - this.start() - } + this.start() const sub = this.emitter.on('did-change', callback) return new Disposable(() => { @@ -91,7 +99,14 @@ class NativeWatcher { return this.emitter.on('should-detach', callback) } - // Private: Register a callback to be invoked when the filesystem watcher has been initialized. + // Private: Register a callback to be invoked when a {NativeWatcher} is about to be stopped. + // + // Returns: A {Disposable} to revoke the subscription. + onWillStop (callback) { + return this.emitter.on('will-stop', callback) + } + + // Private: Register a callback to be invoked when the filesystem watcher has been stopped. // // Returns: A {Disposable} to revoke the subscription. onDidStop (callback) { @@ -108,12 +123,15 @@ class NativeWatcher { // // Has no effect if the watcher is not running. async stop () { - if (!this.running) { + if (this.state !== WATCHER_STATE.RUNNING) { return } - this.running = false + this.state = WATCHER_STATE.STOPPING + this.emitter.emit('will-stop') await this.watcher.stop() + this.state = WATCHER_STATE.STOPPED + this.emitter.emit('did-stop') } @@ -171,6 +189,9 @@ class Watcher { }) }) + this.attachedPromise = new Promise(resolve => { + this.resolveAttachedPromise = resolve + }) this.startPromise = new Promise(resolve => { this.resolveStartPromise = resolve }) @@ -183,6 +204,10 @@ class Watcher { return this.normalizedPathPromise } + getAttachedPromise () { + return this.attachedPromise + } + getStartPromise () { return this.startPromise } @@ -194,7 +219,7 @@ class Watcher { this.native.start() } else { - // Attach and retry + // Attach to a new native listener and retry this.nativeWatcherRegistry.attach(this).then(() => { this.onDidChange(callback) }) @@ -226,15 +251,18 @@ class Watcher { formerSub.dispose() } - if (this.changeCallbacks.size > 0) { - native.start() - } - this.subs.add(native.onShouldDetach(replacement => { if (replacement !== native) { this.attachToNative(replacement) } })) + + this.subs.add(native.onWillStop(() => { + this.subs.dispose() + this.native = null + })) + + this.resolveAttachedPromise() } onNativeEvents (events, callback) { @@ -265,7 +293,7 @@ export default class FileSystemManager { const nativeWatcher = new NativeWatcher(normalizedPath) this.liveWatchers.add(nativeWatcher) - const sub = nativeWatcher.onDidStop(() => { + const sub = nativeWatcher.onWillStop(() => { this.liveWatchers.delete(nativeWatcher) sub.dispose() }) diff --git a/src/native-watcher-registry.js b/src/native-watcher-registry.js index 169201310..95a5a6483 100644 --- a/src/native-watcher-registry.js +++ b/src/native-watcher-registry.js @@ -248,8 +248,8 @@ export default class NativeWatcherRegistry { const leaf = new RegistryWatcherNode(native) this.tree = this.tree.insert(pathSegments, leaf) - const sub = native.onDidStop(() => { - this.tree = this.tree.remove(pathSegments) + const sub = native.onWillStop(() => { + this.tree = this.tree.remove(pathSegments) || new RegistryNode() sub.dispose() })