From 508b56da28c49503a97097b1c8c764bf1027ebfd Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 1 May 2019 17:55:24 -0600 Subject: [PATCH] Remove logic supporting dynamic switching of path watcher backends The logic is pretty complex and I don't want to take the time to integrate @atom/notify with it. I left a bunch of stuff commented out in this commit just in case these changes break the build. I'll do another pass to delete commented code once we go green. --- spec/path-watcher-spec.js | 201 ++++++++++++++++++++------------------ src/path-watcher.js | 147 ++++++++++------------------ 2 files changed, 154 insertions(+), 194 deletions(-) diff --git a/spec/path-watcher-spec.js b/spec/path-watcher-spec.js index edba6f6b8..27ab6dea9 100644 --- a/spec/path-watcher-spec.js +++ b/spec/path-watcher-spec.js @@ -6,7 +6,7 @@ import path from 'path' import { promisify } from 'util' import { CompositeDisposable } from 'event-kit' -import { watchPath, stopAllWatchers } from '../src/path-watcher' +import { PathWatcherManager } from '../src/path-watcher' temp.track() @@ -17,7 +17,7 @@ const realpath = promisify(fs.realpath) const tempMkdir = promisify(temp.mkdir) -describe('watchPath', function () { +describe('PathWatcherManager', function () { let subs beforeEach(function () { @@ -26,7 +26,6 @@ describe('watchPath', function () { afterEach(async function () { subs.dispose() - await stopAllWatchers() }) function waitForChanges (watcher, ...fileNames) { @@ -51,128 +50,136 @@ describe('watchPath', function () { }) } - describe('watchPath()', function () { - beforeEach(() => { - atom.config.set('core.fileSystemWatcher', 'native') + describe('in "native" mode', () => { + let manager + + beforeEach(function () { + manager = new PathWatcherManager('native') }) - it('resolves the returned promise when the watcher begins listening', async function () { - const rootDir = await tempMkdir('atom-fsmanager-test-') - - const watcher = await watchPath(rootDir, {}, () => {}) - expect(watcher.constructor.name).toBe('PathWatcher') + afterEach(async function () { + await manager.stopAllWatchers() }) - it('reuses an existing native watcher and resolves getStartPromise immediately if attached to a running watcher', async function () { - const rootDir = await tempMkdir('atom-fsmanager-test-') + describe('watchPath()', function () { + it('resolves the returned promise when the watcher begins listening', async function () { + const rootDir = await tempMkdir('atom-fsmanager-test-') - const watcher0 = await watchPath(rootDir, {}, () => {}) - const watcher1 = await watchPath(rootDir, {}, () => {}) + const watcher = await manager.watchPath(rootDir, {}, () => {}) + expect(watcher.constructor.name).toBe('PathWatcher') + }) - expect(watcher0.native).toBe(watcher1.native) - }) + it('reuses an existing native watcher and resolves getStartPromise immediately if attached to a running watcher', async function () { + const rootDir = await tempMkdir('atom-fsmanager-test-') - it("reuses existing native watchers even while they're still starting", async function () { - const rootDir = await tempMkdir('atom-fsmanager-test-') + const watcher0 = await manager.watchPath(rootDir, {}, () => {}) + const watcher1 = await manager.watchPath(rootDir, {}, () => {}) - const [watcher0, watcher1] = await Promise.all([ - watchPath(rootDir, {}, () => {}), - watchPath(rootDir, {}, () => {}) - ]) - expect(watcher0.native).toBe(watcher1.native) - }) + expect(watcher0.native).toBe(watcher1.native) + }) - it("doesn't attach new watchers to a native watcher that's stopping", async function () { - const rootDir = await tempMkdir('atom-fsmanager-test-') + it("reuses existing native watchers even while they're still starting", async function () { + const rootDir = await tempMkdir('atom-fsmanager-test-') - const watcher0 = await watchPath(rootDir, {}, () => {}) - const native0 = watcher0.native + const [watcher0, watcher1] = await Promise.all([ + manager.watchPath(rootDir, {}, () => {}), + manager.watchPath(rootDir, {}, () => {}) + ]) + expect(watcher0.native).toBe(watcher1.native) + }) - watcher0.dispose() - const watcher1 = await watchPath(rootDir, {}, () => {}) + it("doesn't attach new watchers to a native watcher that's stopping", async function () { + const rootDir = await tempMkdir('atom-fsmanager-test-') - expect(watcher1.native).not.toBe(native0) - }) + const watcher0 = await manager.watchPath(rootDir, {}, () => {}) + const native0 = watcher0.native - it('reuses an existing native watcher on a parent directory and filters events', async function () { - const rootDir = await tempMkdir('atom-fsmanager-test-').then(realpath) - const rootFile = path.join(rootDir, 'rootfile.txt') - const subDir = path.join(rootDir, 'subdir') - const subFile = path.join(subDir, 'subfile.txt') + watcher0.dispose() + const watcher1 = await manager.watchPath(rootDir, {}, () => {}) - await mkdir(subDir) + expect(watcher1.native).not.toBe(native0) + }) - // Keep the watchers alive with an undisposed subscription - const rootWatcher = await watchPath(rootDir, {}, () => {}) - const childWatcher = await watchPath(subDir, {}, () => {}) + it('reuses an existing native watcher on a parent directory and filters events', async function () { + const rootDir = await tempMkdir('atom-fsmanager-test-').then(realpath) + const rootFile = path.join(rootDir, 'rootfile.txt') + const subDir = path.join(rootDir, 'subdir') + const subFile = path.join(subDir, 'subfile.txt') - expect(rootWatcher.native).toBe(childWatcher.native) - expect(rootWatcher.native.isRunning()).toBe(true) + await mkdir(subDir) - const firstChanges = Promise.all([ - waitForChanges(rootWatcher, subFile), - waitForChanges(childWatcher, subFile) - ]) - await writeFile(subFile, 'subfile\n', { encoding: 'utf8' }) - await firstChanges + // Keep the watchers alive with an undisposed subscription + const rootWatcher = await manager.watchPath(rootDir, {}, () => {}) + const childWatcher = await manager.watchPath(subDir, {}, () => {}) - const nextRootEvent = waitForChanges(rootWatcher, rootFile) - await writeFile(rootFile, 'rootfile\n', { encoding: 'utf8' }) - await nextRootEvent - }) + expect(rootWatcher.native).toBe(childWatcher.native) + expect(rootWatcher.native.isRunning()).toBe(true) - it('adopts existing child watchers and filters events appropriately to them', async function () { - const parentDir = await tempMkdir('atom-fsmanager-test-') - .then(realpath) + const firstChanges = Promise.all([ + waitForChanges(rootWatcher, subFile), + waitForChanges(childWatcher, subFile) + ]) + await writeFile(subFile, 'subfile\n', { encoding: 'utf8' }) + await firstChanges - // Create the directory tree - const rootFile = path.join(parentDir, 'rootfile.txt') - const subDir0 = path.join(parentDir, 'subdir0') - const subFile0 = path.join(subDir0, 'subfile0.txt') - const subDir1 = path.join(parentDir, 'subdir1') - const subFile1 = path.join(subDir1, 'subfile1.txt') + const nextRootEvent = waitForChanges(rootWatcher, rootFile) + await writeFile(rootFile, 'rootfile\n', { encoding: 'utf8' }) + await nextRootEvent + }) - await mkdir(subDir0) - await mkdir(subDir1) - await Promise.all([ - writeFile(rootFile, 'rootfile\n', { encoding: 'utf8' }), - writeFile(subFile0, 'subfile 0\n', { encoding: 'utf8' }), - writeFile(subFile1, 'subfile 1\n', { encoding: 'utf8' }) - ]) + it('adopts existing child watchers and filters events appropriately to them', async function () { + const parentDir = await tempMkdir('atom-fsmanager-test-') + .then(realpath) - // Begin the child watchers and keep them alive - const subWatcher0 = await watchPath(subDir0, {}, () => {}) - const subWatcherChanges0 = waitForChanges(subWatcher0, subFile0) + // Create the directory tree + const rootFile = path.join(parentDir, 'rootfile.txt') + const subDir0 = path.join(parentDir, 'subdir0') + const subFile0 = path.join(subDir0, 'subfile0.txt') + const subDir1 = path.join(parentDir, 'subdir1') + const subFile1 = path.join(subDir1, 'subfile1.txt') - const subWatcher1 = await watchPath(subDir1, {}, () => {}) - const subWatcherChanges1 = waitForChanges(subWatcher1, subFile1) + await mkdir(subDir0) + await mkdir(subDir1) + await Promise.all([ + writeFile(rootFile, 'rootfile\n', { encoding: 'utf8' }), + writeFile(subFile0, 'subfile 0\n', { encoding: 'utf8' }), + writeFile(subFile1, 'subfile 1\n', { encoding: 'utf8' }) + ]) - expect(subWatcher0.native).not.toBe(subWatcher1.native) + // Begin the child watchers and keep them alive + const subWatcher0 = await manager.watchPath(subDir0, {}, () => {}) + const subWatcherChanges0 = waitForChanges(subWatcher0, subFile0) - // Create the parent watcher - const parentWatcher = await watchPath(parentDir, {}, () => {}) - const parentWatcherChanges = waitForChanges( - parentWatcher, - rootFile, - subFile0, - subFile1 - ) + const subWatcher1 = await manager.watchPath(subDir1, {}, () => {}) + const subWatcherChanges1 = waitForChanges(subWatcher1, subFile1) - expect(subWatcher0.native).toBe(parentWatcher.native) - expect(subWatcher1.native).toBe(parentWatcher.native) + expect(subWatcher0.native).not.toBe(subWatcher1.native) - // Ensure events are filtered correctly - await Promise.all([ - appendFile(rootFile, 'change\n', { encoding: 'utf8' }), - appendFile(subFile0, 'change\n', { encoding: 'utf8' }), - appendFile(subFile1, 'change\n', { encoding: 'utf8' }) - ]) + // Create the parent watcher + const parentWatcher = await manager.watchPath(parentDir, {}, () => {}) + const parentWatcherChanges = waitForChanges( + parentWatcher, + rootFile, + subFile0, + subFile1 + ) - await Promise.all([ - subWatcherChanges0, - subWatcherChanges1, - parentWatcherChanges - ]) + expect(subWatcher0.native).toBe(parentWatcher.native) + expect(subWatcher1.native).toBe(parentWatcher.native) + + // Ensure events are filtered correctly + await Promise.all([ + appendFile(rootFile, 'change\n', { encoding: 'utf8' }), + appendFile(subFile0, 'change\n', { encoding: 'utf8' }), + appendFile(subFile1, 'change\n', { encoding: 'utf8' }) + ]) + + await Promise.all([ + subWatcherChanges0, + subWatcherChanges1, + parentWatcherChanges + ]) + }) }) }) }) diff --git a/src/path-watcher.js b/src/path-watcher.js index 93cd9ad6e..b9d0b59ff 100644 --- a/src/path-watcher.js +++ b/src/path-watcher.js @@ -3,7 +3,7 @@ const path = require('path') const {Emitter, Disposable, CompositeDisposable} = require('event-kit') -const Watcher = require('@atom/notify') +const NotifyWatcher = require('@atom/notify') const nsfw = require('@atom/nsfw') const {NativeWatcherRegistry} = require('./native-watcher-registry') @@ -385,12 +385,12 @@ class PathWatcher { return this.normalizedPathPromise } - // Private: Return a {Promise} that will resolve the first time that this watcher is attached to a native watcher. - getAttachedPromise () { - return this.attachedPromise - } + // // Private: Return a {Promise} that will resolve the first time that this watcher is attached to a native watcher. + // getAttachedPromise () { + // return this.attachedPromise + // } - // Extended: Return a {Promise} that will resolve when the underlying native watcher is ready to begin sending events. + // Private: Return a {Promise} that will resolve when the underlying native watcher is ready to begin sending events. // When testing filesystem watchers, it's important to await this promise before making filesystem changes that you // intend to assert about because there will be a delay between the instantiation of the watcher and the activation // of the underlying OS resources that feed its events. @@ -546,50 +546,13 @@ class PathWatcherManager { static active () { if (!this.activeManager) { this.activeManager = new PathWatcherManager(atom.config.get('core.fileSystemWatcher')) - this.sub = atom.config.onDidChange('core.fileSystemWatcher', ({newValue}) => { this.transitionTo(newValue) }) } return this.activeManager } - // Private: Replace the active {PathWatcherManager} with a new one that creates [NativeWatchers]{NativeWatcher} - // based on the value of `setting`. - static async transitionTo (setting) { - const current = this.active() - - if (this.transitionPromise) { - await this.transitionPromise - } - - if (current.setting === setting) { - return - } - current.isShuttingDown = true - - let resolveTransitionPromise = () => {} - this.transitionPromise = new Promise(resolve => { - resolveTransitionPromise = resolve - }) - - const replacement = new PathWatcherManager(setting) - this.activeManager = replacement - - await Promise.all( - Array.from(current.live, async ([root, native]) => { - const w = await replacement.createWatcher(root, {}, () => {}) - native.reattachTo(w.native, root, w.native.options || {}) - }) - ) - - current.stopAllWatchers() - - resolveTransitionPromise() - this.transitionPromise = null - } - // Private: Initialize global {PathWatcher} state. constructor (setting) { this.setting = setting - this.watcher = null this.live = new Map() const initLocal = NativeConstructor => { @@ -610,15 +573,9 @@ class PathWatcherManager { if (setting === 'atom') { initLocal(AtomNativeWatcher) - } else if (setting === 'experimental') { - // - } else if (setting === 'poll') { - // - } else { + } else if (setting === 'native') { initLocal(NSFWNativeWatcher) } - - this.isShuttingDown = false } useExperimentalWatcher () { @@ -626,34 +583,45 @@ class PathWatcherManager { } // Private: Create a {PathWatcher} tied to this global state. See {watchPath} for detailed arguments. - async createWatcher (rootPath, options, eventCallback) { - if (this.isShuttingDown) { - await this.constructor.transitionPromise - return PathWatcherManager.active().createWatcher(rootPath, options, eventCallback) - } - + async watchPath (rootPath, options, eventCallback) { if (this.useExperimentalWatcher()) { - if (!this.watcher) this.watcher = new Watcher() + if (!this.notifyWatcher) this.notifyWatcher = new NotifyWatcher() // TODO: Figure out how to handle the poll setting // if (this.setting === 'poll') { // options.poll = true // } - const watcher = await this.watcher.watchPath(rootPath, eventCallback) - watcher.onDidError = () => {} - return watcher + const w = await this.notifyWatcher.watchPath(rootPath, eventCallback) + w.onDidError = () => {} + return w + } else { + debugger + const w = new PathWatcher(this.nativeRegistry, rootPath, options) + w.onDidChange(eventCallback) + await w.getStartPromise() + return w } + } - const w = new PathWatcher(this.nativeRegistry, rootPath, options) - w.onDidChange(eventCallback) - await w.getStartPromise() - return w + // Private: Stop all living watchers. + // + // Returns a {Promise} that resolves when all native watcher resources are disposed. + stopAllWatchers () { + if (this.useExperimentalWatcher()) { + this.notifyWatcher.kill() + this.notifyWatcher = null; + return Promise.resolve() + } else { + return Promise.all( + Array.from(this.live, ([, w]) => w.stop()) + ) + } } // // Private: Directly access the {NativeWatcherRegistry}. // getRegistry () { - // if (this.useExperimentalWatcher()) { + // if (this.useExperimentalWsatcher()) { // return watcher.getRegistry() // } // @@ -677,21 +645,6 @@ class PathWatcherManager { // // return this.nativeRegistry.print() // } - - // Private: Stop all living watchers. - // - // Returns a {Promise} that resolves when all native watcher resources are disposed. - stopAllWatchers () { - if (this.useExperimentalWatcher()) { - this.watcher.kill() - this.watcher = null - return Promise.resolve() - } else { - return Promise.all( - Array.from(this.live, ([, w]) => w.stop()) - ) - } - } } // Extended: Invoke a callback with each filesystem event that occurs beneath a specified path. If you only need to @@ -734,7 +687,7 @@ class PathWatcherManager { // ``` // function watchPath (rootPath, options, eventCallback) { - return PathWatcherManager.active().createWatcher(rootPath, options, eventCallback) + return PathWatcherManager.active().watchPath(rootPath, options, eventCallback) } // Private: Return a Promise that resolves when all {NativeWatcher} instances associated with a FileSystemManager @@ -743,24 +696,24 @@ function stopAllWatchers () { return PathWatcherManager.active().stopAllWatchers() } -// Private: Show the currently active native watchers in a formatted {String}. -watchPath.printWatchers = function () { - return PathWatcherManager.active().print() -} - -// Private: Access the active {NativeWatcherRegistry}. -watchPath.getRegistry = function () { - return PathWatcherManager.active().getRegistry() -} - -// Private: Sample usage statistics for the active watcher. -watchPath.status = function () { - return PathWatcherManager.active().status() -} - +// // Private: Show the currently active native watchers in a formatted {String}. +// watchPath.printWatchers = function () { +// return PathWatcherManager.active().print() +// } +// +// // Private: Access the active {NativeWatcherRegistry}. +// watchPath.getRegistry = function () { +// return PathWatcherManager.active().getRegistry() +// } +// +// // Private: Sample usage statistics for the active watcher. +// watchPath.status = function () { +// return PathWatcherManager.active().status() +// } +// // // Private: Configure @atom/watcher ("experimental") directly. // watchPath.configure = function (...args) { // return watcher.configure(...args) // } -module.exports = {watchPath, stopAllWatchers} +module.exports = {watchPath, stopAllWatchers, PathWatcherManager}