From 6ffb3f3ff25704a261b856d223ba68b70a367be9 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 17 Aug 2017 21:59:32 -0400 Subject: [PATCH 1/9] Test first --- spec/path-watcher-spec.js | 54 ++++++++++++--------------------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/spec/path-watcher-spec.js b/spec/path-watcher-spec.js index a805886e3..cfa863bf4 100644 --- a/spec/path-watcher-spec.js +++ b/spec/path-watcher-spec.js @@ -48,21 +48,18 @@ describe('watchPath', function () { } describe('watchPath()', function () { - it('resolves getStartPromise() when the watcher begins listening', async function () { + it('resolves the returned promise when the watcher begins listening', async function () { const rootDir = await temp.mkdir('atom-fsmanager-test-') - const watcher = watchPath(rootDir, {}, () => {}) - await watcher.getStartPromise() + const watcher = await watchPath(rootDir, {}, () => {}) + expect(watcher).to.be.a('PathWatcher') }) it('reuses an existing native watcher and resolves getStartPromise immediately if attached to a running watcher', async function () { const rootDir = await temp.mkdir('atom-fsmanager-test-') - const watcher0 = watchPath(rootDir, {}, () => {}) - await watcher0.getStartPromise() - - const watcher1 = watchPath(rootDir, {}, () => {}) - await watcher1.getStartPromise() + const watcher0 = await watchPath(rootDir, {}, () => {}) + const watcher1 = await watchPath(rootDir, {}, () => {}) expect(watcher0.native).toBe(watcher1.native) }) @@ -70,28 +67,21 @@ describe('watchPath', function () { it("reuses existing native watchers even while they're still starting", async function () { const rootDir = await temp.mkdir('atom-fsmanager-test-') - const watcher0 = watchPath(rootDir, {}, () => {}) - await watcher0.getAttachedPromise() - expect(watcher0.native.isRunning()).toBe(false) - - const watcher1 = watchPath(rootDir, {}, () => {}) - await watcher1.getAttachedPromise() - + const [watcher0, watcher1] = await Promise.all([ + watchPath(rootDir, {}, () => {}), + watchPath(rootDir, {}, () => {}) + ]) 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-test-') - const watcher0 = watchPath(rootDir, {}, () => {}) - await watcher0.getStartPromise() + const watcher0 = await watchPath(rootDir, {}, () => {}) const native0 = watcher0.native watcher0.dispose() - - const watcher1 = watchPath(rootDir, {}, () => {}) + const watcher1 = await watchPath(rootDir, {}, () => {}) expect(watcher1.native).not.toBe(native0) }) @@ -105,13 +95,8 @@ describe('watchPath', function () { await fs.mkdir(subDir) // Keep the watchers alive with an undisposed subscription - const rootWatcher = watchPath(rootDir, {}, () => {}) - const childWatcher = watchPath(subDir, {}, () => {}) - - await Promise.all([ - rootWatcher.getStartPromise(), - childWatcher.getStartPromise() - ]) + const rootWatcher = await watchPath(rootDir, {}, () => {}) + const childWatcher = await watchPath(subDir, {}, () => {}) expect(rootWatcher.native).toBe(childWatcher.native) expect(rootWatcher.native.isRunning()).toBe(true) @@ -120,13 +105,11 @@ describe('watchPath', function () { waitForChanges(rootWatcher, subFile), waitForChanges(childWatcher, subFile) ]) - await fs.writeFile(subFile, 'subfile\n', {encoding: 'utf8'}) await firstChanges const nextRootEvent = waitForChanges(rootWatcher, rootFile) await fs.writeFile(rootFile, 'rootfile\n', {encoding: 'utf8'}) - await nextRootEvent }) @@ -149,23 +132,18 @@ describe('watchPath', function () { ]) // Begin the child watchers and keep them alive - const subWatcher0 = watchPath(subDir0, {}, () => {}) + const subWatcher0 = await watchPath(subDir0, {}, () => {}) const subWatcherChanges0 = waitForChanges(subWatcher0, subFile0) - const subWatcher1 = watchPath(subDir1, {}, () => {}) + const subWatcher1 = await watchPath(subDir1, {}, () => {}) const subWatcherChanges1 = waitForChanges(subWatcher1, subFile1) - await Promise.all( - [subWatcher0, subWatcher1].map(watcher => watcher.getStartPromise()) - ) expect(subWatcher0.native).not.toBe(subWatcher1.native) // Create the parent watcher - const parentWatcher = watchPath(parentDir, {}, () => {}) + const parentWatcher = await watchPath(parentDir, {}, () => {}) const parentWatcherChanges = waitForChanges(parentWatcher, rootFile, subFile0, subFile1) - await parentWatcher.getStartPromise() - expect(subWatcher0.native).toBe(parentWatcher.native) expect(subWatcher1.native).toBe(parentWatcher.native) From aa26eba678a6dd634a9165c7eb07b258cfb9cbfd Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 17 Aug 2017 22:01:25 -0400 Subject: [PATCH 2/9] Jasmine, not Mocha --- spec/path-watcher-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/path-watcher-spec.js b/spec/path-watcher-spec.js index cfa863bf4..ae5cd7d75 100644 --- a/spec/path-watcher-spec.js +++ b/spec/path-watcher-spec.js @@ -52,7 +52,7 @@ describe('watchPath', function () { const rootDir = await temp.mkdir('atom-fsmanager-test-') const watcher = await watchPath(rootDir, {}, () => {}) - expect(watcher).to.be.a('PathWatcher') + expect(watcher.constructor.name).toBe('PathWatcher') }) it('reuses an existing native watcher and resolves getStartPromise immediately if attached to a running watcher', async function () { From fb5f197ae7d159a576cea2ec59acd9b5cb1cd83e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 17 Aug 2017 22:47:39 -0400 Subject: [PATCH 3/9] Protect against stale NativeWatcher events --- src/path-watcher.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/path-watcher.js b/src/path-watcher.js index 001b17818..d2e082584 100644 --- a/src/path-watcher.js +++ b/src/path-watcher.js @@ -505,14 +505,16 @@ class PathWatcher { })) this.subs.add(native.onShouldDetach(({replacement, watchedPath}) => { - if (replacement !== native && this.normalizedPath.startsWith(watchedPath)) { + if (this.native === native && replacement !== native && this.normalizedPath.startsWith(watchedPath)) { this.attachToNative(replacement) } })) this.subs.add(native.onWillStop(() => { - this.subs.dispose() - this.native = null + if (this.native === native) { + this.subs.dispose() + this.native = null + } })) this.resolveAttachedPromise() From a0bdc50535fa3036d3145f90b33098015f01c0ac Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 17 Aug 2017 22:48:03 -0400 Subject: [PATCH 4/9] Return a Promise from watchPath --- src/path-watcher.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/path-watcher.js b/src/path-watcher.js index d2e082584..011aa508b 100644 --- a/src/path-watcher.js +++ b/src/path-watcher.js @@ -606,8 +606,8 @@ class PathWatcherManager { // * `path` {String} containing the absolute path to the filesystem entry that was acted upon. // * `oldPath` For rename events, {String} containing the filesystem entry's former absolute path. // -// Returns a {PathWatcher}. Note that every {PathWatcher} is a {Disposable}, so they can be managed by -// [CompositeDisposables]{CompositeDisposable} if desired. +// Returns a {Promise} that will resolve to a {PathWatcher} once it has started. Note that every {PathWatcher} +// is a {Disposable}, so they can be managed by a {CompositeDisposable} if desired. // // ```js // const {watchPath} = require('atom') @@ -631,7 +631,8 @@ class PathWatcherManager { // ``` // function watchPath (rootPath, options, eventCallback) { - return PathWatcherManager.instance().createWatcher(rootPath, options, eventCallback) + const watcher = PathWatcherManager.instance().createWatcher(rootPath, options, eventCallback) + return watcher.getStartPromise().then(() => watcher) } // Private: Return a Promise that resolves when all {NativeWatcher} instances associated with a FileSystemManager From e2c9cc16924d86997bb129eee0cc8d3d955eaade Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 Aug 2017 09:08:34 -0400 Subject: [PATCH 5/9] Private diagnostic method to dump the active watchers --- src/native-watcher-registry.js | 8 ++++++++ src/path-watcher.js | 12 +++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/native-watcher-registry.js b/src/native-watcher-registry.js index a779f78f5..0fffe4f77 100644 --- a/src/native-watcher-registry.js +++ b/src/native-watcher-registry.js @@ -431,6 +431,14 @@ class NativeWatcherRegistry { watcher.attachToNative(native, nativePath) }) } + + // Private: Generate a visual representation of the currently active watchers managed by this + // registry. + // + // Returns a {String} showing the tree structure. + print() { + return this.tree.print() + } } module.exports = {NativeWatcherRegistry} diff --git a/src/path-watcher.js b/src/path-watcher.js index 011aa508b..88f9cd77d 100644 --- a/src/path-watcher.js +++ b/src/path-watcher.js @@ -581,6 +581,11 @@ class PathWatcherManager { return watcher } + // Private: Return a {String} depicting the currently active native watchers. + print () { + return this.nativeRegistry.print() + } + // Private: Stop all living watchers. // // Returns a {Promise} that resolves when all native watcher resources are disposed. @@ -641,4 +646,9 @@ function stopAllWatchers () { return PathWatcherManager.instance().stopAllWatchers() } -module.exports = {watchPath, stopAllWatchers} +// Private: Show the currently active native watchers. +function printWatchers () { + return PathWatcherManager.instance().print() +} + +module.exports = {watchPath, stopAllWatchers, printWatchers} From e7fcb0d079c3a34e0ff5e846f36ee35eb50496e0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 Aug 2017 09:10:20 -0400 Subject: [PATCH 6/9] Adjust Project root directory watching to account for async watchers --- spec/project-spec.coffee | 4 ++-- src/project.coffee | 38 +++++++++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/spec/project-spec.coffee b/spec/project-spec.coffee index 059208cbd..4ce84617a 100644 --- a/spec/project-spec.coffee +++ b/spec/project-spec.coffee @@ -585,10 +585,10 @@ describe "Project", -> waitsForPromise -> stopAllWatchers() runs -> atom.project.setPaths([dirOne]) - waitsForPromise -> atom.project.watchersByPath[dirOne].getStartPromise() + waitsForPromise -> atom.project.getWatcherPromise dirOne runs -> - expect(atom.project.watchersByPath[dirTwo]).toEqual undefined + expect(atom.project.watcherPromisesByPath[dirTwo]).toEqual undefined fs.writeFileSync fileThree, "three\n" fs.writeFileSync fileTwo, "two\n" diff --git a/src/project.coffee b/src/project.coffee index 4564e37bb..2f1b56566 100644 --- a/src/project.coffee +++ b/src/project.coffee @@ -29,13 +29,13 @@ class Project extends Model @repositoryPromisesByPath = new Map() @repositoryProviders = [new GitRepositoryProvider(this, config)] @loadPromisesByPath = {} - @watchersByPath = {} + @watcherPromisesByPath = {} @consumeServices(packageManager) destroyed: -> buffer.destroy() for buffer in @buffers.slice() repository?.destroy() for repository in @repositories.slice() - watcher.dispose() for _, watcher in @watchersByPath + watcher.dispose() for _, watcher in @watcherPromisesByPath @rootDirectories = [] @repositories = [] @@ -212,8 +212,8 @@ class Project extends Model @rootDirectories = [] @repositories = [] - watcher.dispose() for _, watcher in @watchersByPath - @watchersByPath = {} + watcher.then((w) -> w.dispose()) for _, watcher in @watcherPromisesByPath + @watcherPromisesByPath = {} @addPath(projectPath, emitEvent: false) for projectPath in projectPaths @@ -229,11 +229,15 @@ class Project extends Model return if existingDirectory.getPath() is directory.getPath() @rootDirectories.push(directory) - @watchersByPath[directory.getPath()] = watchPath directory.getPath(), {}, (events) => - @emitter.emit 'did-change-files', events + @watcherPromisesByPath[directory.getPath()] = watchPath directory.getPath(), {}, (events) => + # Stop event delivery immediately on removal of a rootDirectory, even if its watcher + # promise has yet to resolve at the time of removal + if @rootDirectories.includes directory + @emitter.emit 'did-change-files', events - for root, watcher in @watchersByPath - watcher.dispose() unless @rootDirectoryies.includes root + for root, watcherPromise in @watcherPromisesByPath + unless @rootDirectories.includes root + watcherPromise.then (watcher) -> watcher.dispose() repo = null for provider in @repositoryProviders @@ -250,6 +254,21 @@ class Project extends Model directory ?= @defaultDirectoryProvider.directoryForURISync(projectPath) directory + # Extended: Access a {Promise} that resolves when the filesystem watcher associated with a project + # root directory is ready to begin receiving events. + # + # This is especially useful in test cases, where it's important to know that the watcher is + # ready before manipulating the filesystem to produce events. + # + # * `projectPath` {String} One of the project's root directories. + # + # Returns a {Promise} that resolves with the {PathWatcher} associated with this project root + # once it has initialized and is ready to start sending events. The Promise will reject with + # an error instead if `projectPath` is not currently a root directory. + getWatcherPromise: (projectPath) -> + @watcherPromisesByPath[projectPath] or + Promise.reject(new Error("#{projectPath} is not a project root")) + # Public: remove a path from the project's list of root paths. # # * `projectPath` {String} The path to remove. @@ -268,7 +287,8 @@ class Project extends Model [removedDirectory] = @rootDirectories.splice(indexToRemove, 1) [removedRepository] = @repositories.splice(indexToRemove, 1) removedRepository?.destroy() unless removedRepository in @repositories - @watchersByPath[projectPath]?.dispose() + @watcherPromisesByPath[projectPath]?.then (w) -> w.dispose() + delete @watcherPromisesByPath[projectPath] @emitter.emit "did-change-paths", @getPaths() true else From ea128849dae91470e525af05f397766aae87684e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 Aug 2017 09:48:53 -0400 Subject: [PATCH 7/9] :shirt: --- src/native-watcher-registry.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/native-watcher-registry.js b/src/native-watcher-registry.js index 0fffe4f77..798ff0619 100644 --- a/src/native-watcher-registry.js +++ b/src/native-watcher-registry.js @@ -436,7 +436,7 @@ class NativeWatcherRegistry { // registry. // // Returns a {String} showing the tree structure. - print() { + print () { return this.tree.print() } } From 4c2d44059cdd05b3a0c38af89dc5d6015f768cc7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 Aug 2017 15:01:21 -0400 Subject: [PATCH 8/9] :arrow_up: github --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 3543aff92..0c8b4609d 100644 --- a/package.json +++ b/package.json @@ -108,7 +108,7 @@ "exception-reporting": "0.41.4", "find-and-replace": "0.209.5", "fuzzy-finder": "1.5.8", - "github": "0.4.2", + "github": "0.5.0", "git-diff": "1.3.6", "go-to-line": "0.32.1", "grammar-selector": "0.49.5", From d03e5d9d54b2af426a742e2d0351e4dda4049169 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 18 Aug 2017 15:02:34 -0400 Subject: [PATCH 9/9] Update documentation references --- src/path-watcher.js | 6 ++++-- src/project.coffee | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/path-watcher.js b/src/path-watcher.js index 88f9cd77d..69bf36da0 100644 --- a/src/path-watcher.js +++ b/src/path-watcher.js @@ -339,7 +339,7 @@ class NativeWatcher { // ```js // const {watchPath} = require('atom') // -// const disposable = watchPath('/var/log', {}, events => { +// const disposable = await watchPath('/var/log', {}, events => { // console.log(`Received batch of ${events.length} events.`) // for (const event of events) { // // "created", "modified", "deleted", "renamed" @@ -424,6 +424,8 @@ class PathWatcher { // 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 it events. // + // PathWatchers acquired through `watchPath` are already started. + // // ```js // const {watchPath} = require('atom') // const ROOT = path.join(__dirname, 'fixtures') @@ -617,7 +619,7 @@ class PathWatcherManager { // ```js // const {watchPath} = require('atom') // -// const disposable = watchPath('/var/log', {}, events => { +// const disposable = await watchPath('/var/log', {}, events => { // console.log(`Received batch of ${events.length} events.`) // for (const event of events) { // // "created", "modified", "deleted", "renamed" diff --git a/src/project.coffee b/src/project.coffee index 2f1b56566..756fd756a 100644 --- a/src/project.coffee +++ b/src/project.coffee @@ -140,6 +140,10 @@ class Project extends Model # # To watch paths outside of open projects, use the `watchPaths` function instead; see {PathWatcher}. # + # When writing tests against functionality that uses this method, be sure to wait for the + # {Promise} returned by {getWatcherPromise()} before manipulating the filesystem to ensure that + # the watcher is receiving events. + # # * `callback` {Function} to be called with batches of filesystem events reported by # the operating system. # * `events` An {Array} of objects that describe a batch of filesystem events.