From e437a74154f50beb24b51e3f1e050e0d3c3f213e Mon Sep 17 00:00:00 2001 From: Dave Combs Date: Tue, 6 Feb 2018 17:16:09 -0800 Subject: [PATCH 1/4] the one-shot version of the commit for the new watchman client wrapper that also reduced the number of direct clients of watchman.Client to 1 per process --- src/watchman_client.js | 286 +++++++++++++++++++--------------------- src/watchman_watcher.js | 41 +++--- 2 files changed, 151 insertions(+), 176 deletions(-) diff --git a/src/watchman_client.js b/src/watchman_client.js index 54e0012..abcff66 100644 --- a/src/watchman_client.js +++ b/src/watchman_client.js @@ -2,10 +2,6 @@ var watchman = require('fb-watchman'); -function values(obj) { - return Object.keys(obj).map(key => obj[key]); -} - /** * Constants */ @@ -17,40 +13,39 @@ function values(obj) { * we're using 'watch-project' or 'watch', what the 'project root' is when * we internally use watch-project, whether a connection has been lost * and reestablished, etc. Also switched to doing things with promises and known-name - * methods in WatchmanWatcher, so as much information as possible can be kept in + * methods in WatchmanWatcher, so as much information as possible can be kept in * the WatchmanClient, ultimately making this the only object listening directly - * to watchman.Client, then forwarding appropriately (via the known-name methods) to + * to watchman.Client, then forwarding appropriately (via the known-name methods) to * the relevant WatchmanWatcher(s). - * + * * Note: WatchmanWatcher also added a 'watchmanPath' option for use with the sane CLI. * Because of that, we actually need a map of watchman binary path to WatchmanClient instance. * That is set up in getInstance(). Once the WatchmanWatcher has a given client, it doesn't * change. * * @class WatchmanClient - * @param String watchmanBinaryPath + * @param String watchmanBinaryPath * @public */ function WatchmanClient(watchmanBinaryPath) { + // define/clear some local state. The properties will be initialized // in _handleClientAndCheck(). This is also called again in _onEnd when // trying to reestablish connection to watchman. - this._clearLocalVars(); - + this._clearLocalVars(); + this._watchmanBinaryPath = watchmanBinaryPath; this._backoffTimes = this._setupBackoffTimes(); - this._clientListeners = null; // direct listeners from here to watchman.Client. + this._clientListeners = null; // direct listeners from here to watchman.Client. } // Define 'wildmatch' property, which must be available when we call the -// WatchmanWatcher.createOptions() method. +// WatchmanWatcher.createOptions() method. Object.defineProperty(WatchmanClient.prototype, 'wildmatch', { - get() { - return this._wildmatch; - }, + get() { return this._wildmatch; } }); /** @@ -61,16 +56,16 @@ Object.defineProperty(WatchmanClient.prototype, 'wildmatch', { WatchmanClient.prototype.subscribe = function(watchmanWatcher, root) { let subscription; let watcherInfo; - + return this._setupClient() .then(() => { watcherInfo = this._createWatcherInfo(watchmanWatcher); subscription = watcherInfo.subscription; return this._watch(subscription, root); }) - .then(() => this._clock(subscription)) - .then(() => this._subscribe(subscription)); - // Note: callers are responsible for noting any subscription failure. + .then((resp) => this._clock(subscription)) + .then((data) => this._subscribe(subscription)); + // Note: callers are responsible for noting any subscription failure. }; /** @@ -79,12 +74,12 @@ WatchmanClient.prototype.subscribe = function(watchmanWatcher, root) { * which will end the connection to the watchman.Client, too. */ WatchmanClient.prototype.closeWatcher = function(watchmanWatcher) { - let watcherInfos = values(this._watcherMap); + let watcherInfos = Object.values(this._watcherMap); let numWatchers = watcherInfos.length; if (numWatchers > 0) { let watcherInfo; - + for (let info of watcherInfos) { if (info.watchmanWatcher === watchmanWatcher) { watcherInfo = info; @@ -98,7 +93,7 @@ WatchmanClient.prototype.closeWatcher = function(watchmanWatcher) { numWatchers--; if (numWatchers === 0) { - this._clearLocalVars(); // nobody watching, so shut the watchman.Client down. + this._clearLocalVars(); // nobody watching, so shut the watchman.Client down. } } } @@ -116,7 +111,7 @@ WatchmanClient.prototype._setupBackoffTimes = function() { _next: 0, next() { - let val = this._times[this._next]; + let val = this._times[_next]; if (this._next < this._times.length - 1) { this._next++; } @@ -125,14 +120,14 @@ WatchmanClient.prototype._setupBackoffTimes = function() { reset() { this._next = 0; - }, - }; -}; + } + } +} /** * Set up the connection to the watchman client. Return a promise * that is fulfilled when we have a client that has finished the - * capabilityCheck. + * capabilityCheck. */ WatchmanClient.prototype._setupClient = function() { if (!this._clientPromise) { @@ -153,10 +148,9 @@ WatchmanClient.prototype._setupClient = function() { */ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { this._createClientAndCheck().then( - value => { - let resp = value.resp; - let client = value.client; - + (value) => { + let {resp, client} = value; + try { this._wildmatch = resp.capabilities.wildmatch; this._relative_root = resp.capabilities.relative_root; @@ -175,7 +169,7 @@ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { reject(error); } }, - () => { + (error) => { // create & capability check failed in any of several ways, // do the retry with backoff. @@ -193,7 +187,7 @@ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { }, backoffMillis); } ); -}; +} /** * Create a promise that will only be fulfilled when either @@ -203,15 +197,14 @@ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { */ WatchmanClient.prototype._createClientAndCheck = function() { return new Promise((resolve, reject) => { + let client; - + try { - client = new watchman.Client( - this._watchmanBinaryPath - ? { watchmanBinaryPath: this._watchmanBinaryPath } - : {} - ); - } catch (error) { + client = new watchman.Client(this._watchmanBinaryPath + ? { watchmanBinaryPath: this._watchmanBinaryPath} + : {}); + } catch(error) { // if we're here, either the binary path is bad or something // else really bad happened. The client doesn't even attempt // to connect until we send it a command, though. @@ -219,7 +212,7 @@ WatchmanClient.prototype._createClientAndCheck = function() { return; } - client.on('error', error => { + client.on('error', (error) => { client.removeAllListeners(); reject(error); }); @@ -230,7 +223,7 @@ WatchmanClient.prototype._createClientAndCheck = function() { }); client.capabilityCheck( - { optional: ['wildmatch', 'relative_root'] }, + {optional: ['wildmatch', 'relative_root']}, (error, resp) => { try { client.removeAllListeners(); @@ -238,7 +231,7 @@ WatchmanClient.prototype._createClientAndCheck = function() { if (error) { reject(error); } else { - resolve({ resp, client }); + resolve({resp, client}); } } catch (err) { // In case we get something weird in the block using 'resp'. @@ -247,9 +240,9 @@ WatchmanClient.prototype._createClientAndCheck = function() { reject(err); } } - ); + ) }); -}; +} /** * Clear out local state at the beginning and if we end up @@ -277,17 +270,17 @@ WatchmanClient.prototype._genSubscription = function() { }; /** - * Create a new watcherInfo entry for the given watchmanWatcher and + * Create a new watcherInfo entry for the given watchmanWatcher and * initialize it. */ WatchmanClient.prototype._createWatcherInfo = function(watchmanWatcher) { let watcherInfo = { subscription: this._genSubscription(), - watchmanWatcher: watchmanWatcher, - root: null, // set during 'watch' or 'watch-project' - relativePath: null, // same - since: null, // set during 'clock' - options: null, // created and set during 'subscribe'. + watchmanWatcher: watchmanWatcher, + root: null, // set during 'watch' or 'watch-project' + relativePath: null, // same + since: null, // set during 'clock' + options: null // created and set during 'subscribe'. }; this._watcherMap[watcherInfo.subscription] = watcherInfo; @@ -300,7 +293,7 @@ WatchmanClient.prototype._createWatcherInfo = function(watchmanWatcher) { */ WatchmanClient.prototype._getWatcherInfo = function(subscription) { return this._watcherMap[subscription]; -}; +} /** * Given a watchmanWatcher and a root, issue the correct 'watch' @@ -309,35 +302,38 @@ WatchmanClient.prototype._getWatcherInfo = function(subscription) { * of the 'watch' or 'watch-project' here. */ WatchmanClient.prototype._watch = function(subscription, root) { + return new Promise((resolve, reject) => { let watcherInfo = this._getWatcherInfo(subscription); - if (this._relative_root) { - this._client.command(['watch-project', root], (error, resp) => { - if (error) { - reject(error); - } else { - watcherInfo.root = resp.watch; - watcherInfo.relativePath = resp.relative_path - ? resp.relative_path - : ''; - resolve(resp); - } - }); + if (this._relative_root) { + this._client.command(['watch-project', root], + (error, resp) => { + if (error) { + reject(error); + } else { + watcherInfo.root = resp.watch; + watcherInfo.relativePath = resp.relative_path + ? resp.relative_path + : ''; + resolve(resp); + } + }); } else { - this._client.command(['watch', root], (error, resp) => { - if (error) { - reject(error); - } else { - watcherInfo.root = root; - watcherInfo.relativePath = ''; - resolve(resp); - } - }); + this._client.command(['watch', root], + (error, resp) => { + if (error) { + reject(error); + } else { + watcherInfo.root = root; + watcherInfo.relativePath = ''; + resolve(resp); + } + }); } }); }; - + /** * Issue the 'clock' command to get the time value for use with the 'since' * option during 'subscribe'. @@ -346,18 +342,19 @@ WatchmanClient.prototype._clock = function(subscription) { return new Promise((resolve, reject) => { let watcherInfo = this._getWatcherInfo(subscription); - this._client.command(['clock', watcherInfo.root], (error, resp) => { - if (error) { - reject(error); - } else { - watcherInfo.since = resp.clock; - resolve(resp); - } - }); + this._client.command(['clock', watcherInfo.root], + (error, resp) => { + if (error) { + reject(error); + } else { + watcherInfo.since = resp.clock; + resolve(resp); + } + }); }); }; -/** +/** * Do the internal handling of calling the watchman.Client for * a subscription. */ @@ -368,9 +365,9 @@ WatchmanClient.prototype._subscribe = function(subscription) { // create the 'bare' options w/o 'since' or relative_root. // Store in watcherInfo for later use if we need to reset // things after an 'end' caught here. - let options = watcherInfo.watchmanWatcher.createOptions(); + let options = watcherInfo.watchmanWatcher.createOptions(); watcherInfo.options = options; - + // Dup the options object so we can add 'relative_root' and 'since' // and leave the original options object alone. We'll do this again // later if we need to resubscribe after 'end' and reconnect. @@ -382,21 +379,19 @@ WatchmanClient.prototype._subscribe = function(subscription) { options.since = watcherInfo.since; - this._client.command( - ['subscribe', watcherInfo.root, subscription, options], - (error, resp) => { - if (error) { - reject(error); - } else { - resolve(resp); - } - } - ); + this._client.command(['subscribe', watcherInfo.root, subscription, options], + (error, resp) => { + if (error) { + reject(error); + } else { + resolve(resp); + } + }); }); -}; +} /** - * Handle the 'subscription' (file change) event, by calling the + * Handle the 'subscription' (file change) event, by calling the * handler on the relevant WatchmanWatcher. */ WatchmanClient.prototype._onSubscription = function(resp) { @@ -408,12 +403,9 @@ WatchmanClient.prototype._onSubscription = function(resp) { watcherInfo.watchmanWatcher.handleChangeEvent(resp); } else { // Note it in the log, but otherwise ignore it - console.error( - "WatchmanClient error - received 'subscription' event " + - "for non-existent subscription '" + - resp.subscription + - "'" - ); + console.error("WatchmanClient error - received 'subscription' event " + + "for non-existent subscription '" + + resp.subscription + "'"); } }; @@ -424,9 +416,8 @@ WatchmanClient.prototype._onSubscription = function(resp) { * which subscription it belonged to). */ WatchmanClient.prototype._onError = function(error) { - values(this._watcherMap).forEach(watcherInfo => - watcherInfo.watchmanWatcher.handleErrorEvent(error) - ); + Object.values(this._watcherMap).forEach((watcherInfo) => + watcherInfo.watchmanWatcher.handleErrorEvent(error)); }; /** @@ -438,56 +429,47 @@ WatchmanClient.prototype._onError = function(error) { * call the error handler on each existing WatchmanWatcher. */ WatchmanClient.prototype._onEnd = function() { - console.warn( - '[sane.WatchmanClient] Warning: Lost connection to watchman, reconnecting..' - ); + console.warn('[sane.WatchmanClient] Warning: Lost connection to watchman, reconnecting..'); // Hold the old watcher map so we use it to recreate all subscriptions. - let oldWatcherInfos = values(this._watcherMap); + let oldWatcherInfos = Object.values(this._watcherMap); this._clearLocalVars(); - - this._setupClient().then( - () => { - let promises = oldWatcherInfos.map(watcherInfo => - this.subscribe( - watcherInfo.watchmanWatcher, - watcherInfo.watchmanWatcher.root - ) - ); - Promise.all(promises).then( - () => { - console.log('[sane.WatchmanClient]: Reconnected to watchman'); - }, - error => { - console.error( - '[sane.WatchmanClient]: Reconnected to watchman, but failed to ' + - 'reestablish at least one subscription, cannot continue' - ); - console.error(error); - oldWatcherInfos.forEach(watcherInfo => - watcherInfo.watchmanWatcher.handleErrorEvent(error) - ); - // XXX not sure whether to clear all _watcherMap instances here, - // but basically this client is inconsistent now, since at least one - // subscribe failed. - } - ); - }, - error => { - console.error( - '[sane.WatchmanClient]: Lost connection to watchman, ' + - 'reconnect failed, cannot continue' - ); - console.error(error); - oldWatcherInfos.forEach(watcherInfo => - watcherInfo.watchmanWatcher.handleErrorEvent(error) - ); - } - ); + + this._setupClient() + .then( + (client) => { + let promises = oldWatcherInfos.map((watcherInfo) => + this.subscribe(watcherInfo.watchmanWatcher, + watcherInfo.watchmanWatcher.root)); + Promise.all(promises) + .then( + (resultArray) => { + console.log('[sane.WatchmanClient]: Reconnected to watchman'); + }, + (error) => { + console.error("[sane.WatchmanClient]: Reconnected to watchman, but failed to " + + "reestablish at least one subscription, cannot continue"); + console.error(error); + oldWatcherInfos.forEach((watcherInfo) => + watcherInfo.watchmanWatcher.handleErrorEvent(error)); + // XXX not sure whether to clear all _watcherMap instances here, + // but basically this client is inconsistent now, since at least one + // subscribe failed. + }); + }, + (error) => { + console.error('[sane.WatchmanClient]: Lost connection to watchman, ' + + 'reconnect failed, cannot continue'); + console.error(error); + oldWatcherInfos.forEach((watcherInfo) => + watcherInfo.watchmanWatcher.handleErrorEvent(error)); + }); }; + module.exports = { + /** * Create/retrieve an instance of the WatchmanClient. See the header comment * about the map of client instances, one per watchmanPath. @@ -503,7 +485,7 @@ module.exports = { } if (watchmanBinaryPath == undefined || watchmanBinaryPath === null) { - watchmanBinaryPath = ''; + watchmanBinaryPath = ""; } let watchmanClient = clientMap[watchmanBinaryPath]; @@ -514,5 +496,5 @@ module.exports = { } return watchmanClient; - }, -}; + } +} diff --git a/src/watchman_watcher.js b/src/watchman_watcher.js index b315155..80e0ac1 100644 --- a/src/watchman_watcher.js +++ b/src/watchman_watcher.js @@ -53,27 +53,27 @@ WatchmanWatcher.prototype._init = function() { // Then subscribe, which will do the appropriate setup so that we will receive // calls to handleChangeEvent when files change. this._client = watchmanClient.getInstance(this.watchmanPath); - - return this._client.subscribe(this, this.root).then( - resp => { - this._handleWarning(resp); - this.emit('ready'); - }, - error => { - this._handleError(error); - } - ); + + return this._client.subscribe(this, this.root) + .then( + (resp) => { + this._handleWarning(resp); + this.emit('ready'); + }, + (error) => { + this._handleError(error); + }); }; /** * Called by WatchmanClient to create the options, either during initial 'subscribe' - * or to resubscribe after a disconnect+reconnect. Note that we are leaving out - * the watchman 'since' and 'relative_root' options, which are handled inside the + * or to resubscribe after a disconnect+reconnect. Note that we are leaving out + * the watchman 'since' and 'relative_root' options, which are handled inside the * WatchmanClient. */ WatchmanWatcher.prototype.createOptions = function() { let options = { - fields: ['name', 'exists', 'new'], + fields: ['name', 'exists', 'new'] }; // If the server has the wildmatch capability available it supports @@ -115,7 +115,7 @@ WatchmanWatcher.prototype.createOptions = function() { /** * Called by WatchmanClient when it receives an error from the watchman daemon. * - * @param {Object} resp + * @param {Object} error */ WatchmanWatcher.prototype.handleErrorEvent = function(error) { this.emit('error', error); @@ -149,10 +149,8 @@ WatchmanWatcher.prototype.handleFileChange = function(changeDescriptor) { relativePath = changeDescriptor.name; absPath = path.join(this.root, relativePath); - if ( - !(this._client.wildmatch && !this.hasIgnore) && - !common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath) - ) { + if (!(this._client.wildmatch && !this.hasIgnore) && + !common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath)) { return; } @@ -190,12 +188,7 @@ WatchmanWatcher.prototype.handleFileChange = function(changeDescriptor) { * @private */ -WatchmanWatcher.prototype.emitEvent = function( - eventType, - filepath, - root, - stat -) { +WatchmanWatcher.prototype.emitEvent = function(eventType, filepath, root, stat) { this.emit(eventType, filepath, root, stat); this.emit(ALL_EVENT, eventType, filepath, root, stat); }; From ae00d265ec34f0ac2c5f02b3f7bddef9e050ab08 Mon Sep 17 00:00:00 2001 From: Dave Combs Date: Tue, 6 Feb 2018 17:54:05 -0800 Subject: [PATCH 2/4] copied from amasad/sane to match auto-formatting and pick up Stef bugfix for _next in backoff timer --- src/watchman_client.js | 286 +++++++++++++++++++++------------------- src/watchman_watcher.js | 41 +++--- 2 files changed, 176 insertions(+), 151 deletions(-) diff --git a/src/watchman_client.js b/src/watchman_client.js index abcff66..54e0012 100644 --- a/src/watchman_client.js +++ b/src/watchman_client.js @@ -2,6 +2,10 @@ var watchman = require('fb-watchman'); +function values(obj) { + return Object.keys(obj).map(key => obj[key]); +} + /** * Constants */ @@ -13,39 +17,40 @@ var watchman = require('fb-watchman'); * we're using 'watch-project' or 'watch', what the 'project root' is when * we internally use watch-project, whether a connection has been lost * and reestablished, etc. Also switched to doing things with promises and known-name - * methods in WatchmanWatcher, so as much information as possible can be kept in + * methods in WatchmanWatcher, so as much information as possible can be kept in * the WatchmanClient, ultimately making this the only object listening directly - * to watchman.Client, then forwarding appropriately (via the known-name methods) to + * to watchman.Client, then forwarding appropriately (via the known-name methods) to * the relevant WatchmanWatcher(s). - * + * * Note: WatchmanWatcher also added a 'watchmanPath' option for use with the sane CLI. * Because of that, we actually need a map of watchman binary path to WatchmanClient instance. * That is set up in getInstance(). Once the WatchmanWatcher has a given client, it doesn't * change. * * @class WatchmanClient - * @param String watchmanBinaryPath + * @param String watchmanBinaryPath * @public */ function WatchmanClient(watchmanBinaryPath) { - // define/clear some local state. The properties will be initialized // in _handleClientAndCheck(). This is also called again in _onEnd when // trying to reestablish connection to watchman. - this._clearLocalVars(); - + this._clearLocalVars(); + this._watchmanBinaryPath = watchmanBinaryPath; this._backoffTimes = this._setupBackoffTimes(); - this._clientListeners = null; // direct listeners from here to watchman.Client. + this._clientListeners = null; // direct listeners from here to watchman.Client. } // Define 'wildmatch' property, which must be available when we call the -// WatchmanWatcher.createOptions() method. +// WatchmanWatcher.createOptions() method. Object.defineProperty(WatchmanClient.prototype, 'wildmatch', { - get() { return this._wildmatch; } + get() { + return this._wildmatch; + }, }); /** @@ -56,16 +61,16 @@ Object.defineProperty(WatchmanClient.prototype, 'wildmatch', { WatchmanClient.prototype.subscribe = function(watchmanWatcher, root) { let subscription; let watcherInfo; - + return this._setupClient() .then(() => { watcherInfo = this._createWatcherInfo(watchmanWatcher); subscription = watcherInfo.subscription; return this._watch(subscription, root); }) - .then((resp) => this._clock(subscription)) - .then((data) => this._subscribe(subscription)); - // Note: callers are responsible for noting any subscription failure. + .then(() => this._clock(subscription)) + .then(() => this._subscribe(subscription)); + // Note: callers are responsible for noting any subscription failure. }; /** @@ -74,12 +79,12 @@ WatchmanClient.prototype.subscribe = function(watchmanWatcher, root) { * which will end the connection to the watchman.Client, too. */ WatchmanClient.prototype.closeWatcher = function(watchmanWatcher) { - let watcherInfos = Object.values(this._watcherMap); + let watcherInfos = values(this._watcherMap); let numWatchers = watcherInfos.length; if (numWatchers > 0) { let watcherInfo; - + for (let info of watcherInfos) { if (info.watchmanWatcher === watchmanWatcher) { watcherInfo = info; @@ -93,7 +98,7 @@ WatchmanClient.prototype.closeWatcher = function(watchmanWatcher) { numWatchers--; if (numWatchers === 0) { - this._clearLocalVars(); // nobody watching, so shut the watchman.Client down. + this._clearLocalVars(); // nobody watching, so shut the watchman.Client down. } } } @@ -111,7 +116,7 @@ WatchmanClient.prototype._setupBackoffTimes = function() { _next: 0, next() { - let val = this._times[_next]; + let val = this._times[this._next]; if (this._next < this._times.length - 1) { this._next++; } @@ -120,14 +125,14 @@ WatchmanClient.prototype._setupBackoffTimes = function() { reset() { this._next = 0; - } - } -} + }, + }; +}; /** * Set up the connection to the watchman client. Return a promise * that is fulfilled when we have a client that has finished the - * capabilityCheck. + * capabilityCheck. */ WatchmanClient.prototype._setupClient = function() { if (!this._clientPromise) { @@ -148,9 +153,10 @@ WatchmanClient.prototype._setupClient = function() { */ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { this._createClientAndCheck().then( - (value) => { - let {resp, client} = value; - + value => { + let resp = value.resp; + let client = value.client; + try { this._wildmatch = resp.capabilities.wildmatch; this._relative_root = resp.capabilities.relative_root; @@ -169,7 +175,7 @@ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { reject(error); } }, - (error) => { + () => { // create & capability check failed in any of several ways, // do the retry with backoff. @@ -187,7 +193,7 @@ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { }, backoffMillis); } ); -} +}; /** * Create a promise that will only be fulfilled when either @@ -197,14 +203,15 @@ WatchmanClient.prototype._handleClientAndCheck = function(resolve, reject) { */ WatchmanClient.prototype._createClientAndCheck = function() { return new Promise((resolve, reject) => { - let client; - + try { - client = new watchman.Client(this._watchmanBinaryPath - ? { watchmanBinaryPath: this._watchmanBinaryPath} - : {}); - } catch(error) { + client = new watchman.Client( + this._watchmanBinaryPath + ? { watchmanBinaryPath: this._watchmanBinaryPath } + : {} + ); + } catch (error) { // if we're here, either the binary path is bad or something // else really bad happened. The client doesn't even attempt // to connect until we send it a command, though. @@ -212,7 +219,7 @@ WatchmanClient.prototype._createClientAndCheck = function() { return; } - client.on('error', (error) => { + client.on('error', error => { client.removeAllListeners(); reject(error); }); @@ -223,7 +230,7 @@ WatchmanClient.prototype._createClientAndCheck = function() { }); client.capabilityCheck( - {optional: ['wildmatch', 'relative_root']}, + { optional: ['wildmatch', 'relative_root'] }, (error, resp) => { try { client.removeAllListeners(); @@ -231,7 +238,7 @@ WatchmanClient.prototype._createClientAndCheck = function() { if (error) { reject(error); } else { - resolve({resp, client}); + resolve({ resp, client }); } } catch (err) { // In case we get something weird in the block using 'resp'. @@ -240,9 +247,9 @@ WatchmanClient.prototype._createClientAndCheck = function() { reject(err); } } - ) + ); }); -} +}; /** * Clear out local state at the beginning and if we end up @@ -270,17 +277,17 @@ WatchmanClient.prototype._genSubscription = function() { }; /** - * Create a new watcherInfo entry for the given watchmanWatcher and + * Create a new watcherInfo entry for the given watchmanWatcher and * initialize it. */ WatchmanClient.prototype._createWatcherInfo = function(watchmanWatcher) { let watcherInfo = { subscription: this._genSubscription(), - watchmanWatcher: watchmanWatcher, - root: null, // set during 'watch' or 'watch-project' - relativePath: null, // same - since: null, // set during 'clock' - options: null // created and set during 'subscribe'. + watchmanWatcher: watchmanWatcher, + root: null, // set during 'watch' or 'watch-project' + relativePath: null, // same + since: null, // set during 'clock' + options: null, // created and set during 'subscribe'. }; this._watcherMap[watcherInfo.subscription] = watcherInfo; @@ -293,7 +300,7 @@ WatchmanClient.prototype._createWatcherInfo = function(watchmanWatcher) { */ WatchmanClient.prototype._getWatcherInfo = function(subscription) { return this._watcherMap[subscription]; -} +}; /** * Given a watchmanWatcher and a root, issue the correct 'watch' @@ -302,38 +309,35 @@ WatchmanClient.prototype._getWatcherInfo = function(subscription) { * of the 'watch' or 'watch-project' here. */ WatchmanClient.prototype._watch = function(subscription, root) { - return new Promise((resolve, reject) => { let watcherInfo = this._getWatcherInfo(subscription); - if (this._relative_root) { - this._client.command(['watch-project', root], - (error, resp) => { - if (error) { - reject(error); - } else { - watcherInfo.root = resp.watch; - watcherInfo.relativePath = resp.relative_path - ? resp.relative_path - : ''; - resolve(resp); - } - }); + if (this._relative_root) { + this._client.command(['watch-project', root], (error, resp) => { + if (error) { + reject(error); + } else { + watcherInfo.root = resp.watch; + watcherInfo.relativePath = resp.relative_path + ? resp.relative_path + : ''; + resolve(resp); + } + }); } else { - this._client.command(['watch', root], - (error, resp) => { - if (error) { - reject(error); - } else { - watcherInfo.root = root; - watcherInfo.relativePath = ''; - resolve(resp); - } - }); + this._client.command(['watch', root], (error, resp) => { + if (error) { + reject(error); + } else { + watcherInfo.root = root; + watcherInfo.relativePath = ''; + resolve(resp); + } + }); } }); }; - + /** * Issue the 'clock' command to get the time value for use with the 'since' * option during 'subscribe'. @@ -342,19 +346,18 @@ WatchmanClient.prototype._clock = function(subscription) { return new Promise((resolve, reject) => { let watcherInfo = this._getWatcherInfo(subscription); - this._client.command(['clock', watcherInfo.root], - (error, resp) => { - if (error) { - reject(error); - } else { - watcherInfo.since = resp.clock; - resolve(resp); - } - }); + this._client.command(['clock', watcherInfo.root], (error, resp) => { + if (error) { + reject(error); + } else { + watcherInfo.since = resp.clock; + resolve(resp); + } + }); }); }; -/** +/** * Do the internal handling of calling the watchman.Client for * a subscription. */ @@ -365,9 +368,9 @@ WatchmanClient.prototype._subscribe = function(subscription) { // create the 'bare' options w/o 'since' or relative_root. // Store in watcherInfo for later use if we need to reset // things after an 'end' caught here. - let options = watcherInfo.watchmanWatcher.createOptions(); + let options = watcherInfo.watchmanWatcher.createOptions(); watcherInfo.options = options; - + // Dup the options object so we can add 'relative_root' and 'since' // and leave the original options object alone. We'll do this again // later if we need to resubscribe after 'end' and reconnect. @@ -379,19 +382,21 @@ WatchmanClient.prototype._subscribe = function(subscription) { options.since = watcherInfo.since; - this._client.command(['subscribe', watcherInfo.root, subscription, options], - (error, resp) => { - if (error) { - reject(error); - } else { - resolve(resp); - } - }); + this._client.command( + ['subscribe', watcherInfo.root, subscription, options], + (error, resp) => { + if (error) { + reject(error); + } else { + resolve(resp); + } + } + ); }); -} +}; /** - * Handle the 'subscription' (file change) event, by calling the + * Handle the 'subscription' (file change) event, by calling the * handler on the relevant WatchmanWatcher. */ WatchmanClient.prototype._onSubscription = function(resp) { @@ -403,9 +408,12 @@ WatchmanClient.prototype._onSubscription = function(resp) { watcherInfo.watchmanWatcher.handleChangeEvent(resp); } else { // Note it in the log, but otherwise ignore it - console.error("WatchmanClient error - received 'subscription' event " + - "for non-existent subscription '" + - resp.subscription + "'"); + console.error( + "WatchmanClient error - received 'subscription' event " + + "for non-existent subscription '" + + resp.subscription + + "'" + ); } }; @@ -416,8 +424,9 @@ WatchmanClient.prototype._onSubscription = function(resp) { * which subscription it belonged to). */ WatchmanClient.prototype._onError = function(error) { - Object.values(this._watcherMap).forEach((watcherInfo) => - watcherInfo.watchmanWatcher.handleErrorEvent(error)); + values(this._watcherMap).forEach(watcherInfo => + watcherInfo.watchmanWatcher.handleErrorEvent(error) + ); }; /** @@ -429,47 +438,56 @@ WatchmanClient.prototype._onError = function(error) { * call the error handler on each existing WatchmanWatcher. */ WatchmanClient.prototype._onEnd = function() { - console.warn('[sane.WatchmanClient] Warning: Lost connection to watchman, reconnecting..'); + console.warn( + '[sane.WatchmanClient] Warning: Lost connection to watchman, reconnecting..' + ); // Hold the old watcher map so we use it to recreate all subscriptions. - let oldWatcherInfos = Object.values(this._watcherMap); + let oldWatcherInfos = values(this._watcherMap); this._clearLocalVars(); - - this._setupClient() - .then( - (client) => { - let promises = oldWatcherInfos.map((watcherInfo) => - this.subscribe(watcherInfo.watchmanWatcher, - watcherInfo.watchmanWatcher.root)); - Promise.all(promises) - .then( - (resultArray) => { - console.log('[sane.WatchmanClient]: Reconnected to watchman'); - }, - (error) => { - console.error("[sane.WatchmanClient]: Reconnected to watchman, but failed to " + - "reestablish at least one subscription, cannot continue"); - console.error(error); - oldWatcherInfos.forEach((watcherInfo) => - watcherInfo.watchmanWatcher.handleErrorEvent(error)); - // XXX not sure whether to clear all _watcherMap instances here, - // but basically this client is inconsistent now, since at least one - // subscribe failed. - }); - }, - (error) => { - console.error('[sane.WatchmanClient]: Lost connection to watchman, ' + - 'reconnect failed, cannot continue'); - console.error(error); - oldWatcherInfos.forEach((watcherInfo) => - watcherInfo.watchmanWatcher.handleErrorEvent(error)); - }); -}; + this._setupClient().then( + () => { + let promises = oldWatcherInfos.map(watcherInfo => + this.subscribe( + watcherInfo.watchmanWatcher, + watcherInfo.watchmanWatcher.root + ) + ); + Promise.all(promises).then( + () => { + console.log('[sane.WatchmanClient]: Reconnected to watchman'); + }, + error => { + console.error( + '[sane.WatchmanClient]: Reconnected to watchman, but failed to ' + + 'reestablish at least one subscription, cannot continue' + ); + console.error(error); + oldWatcherInfos.forEach(watcherInfo => + watcherInfo.watchmanWatcher.handleErrorEvent(error) + ); + // XXX not sure whether to clear all _watcherMap instances here, + // but basically this client is inconsistent now, since at least one + // subscribe failed. + } + ); + }, + error => { + console.error( + '[sane.WatchmanClient]: Lost connection to watchman, ' + + 'reconnect failed, cannot continue' + ); + console.error(error); + oldWatcherInfos.forEach(watcherInfo => + watcherInfo.watchmanWatcher.handleErrorEvent(error) + ); + } + ); +}; module.exports = { - /** * Create/retrieve an instance of the WatchmanClient. See the header comment * about the map of client instances, one per watchmanPath. @@ -485,7 +503,7 @@ module.exports = { } if (watchmanBinaryPath == undefined || watchmanBinaryPath === null) { - watchmanBinaryPath = ""; + watchmanBinaryPath = ''; } let watchmanClient = clientMap[watchmanBinaryPath]; @@ -496,5 +514,5 @@ module.exports = { } return watchmanClient; - } -} + }, +}; diff --git a/src/watchman_watcher.js b/src/watchman_watcher.js index 80e0ac1..b315155 100644 --- a/src/watchman_watcher.js +++ b/src/watchman_watcher.js @@ -53,27 +53,27 @@ WatchmanWatcher.prototype._init = function() { // Then subscribe, which will do the appropriate setup so that we will receive // calls to handleChangeEvent when files change. this._client = watchmanClient.getInstance(this.watchmanPath); - - return this._client.subscribe(this, this.root) - .then( - (resp) => { - this._handleWarning(resp); - this.emit('ready'); - }, - (error) => { - this._handleError(error); - }); + + return this._client.subscribe(this, this.root).then( + resp => { + this._handleWarning(resp); + this.emit('ready'); + }, + error => { + this._handleError(error); + } + ); }; /** * Called by WatchmanClient to create the options, either during initial 'subscribe' - * or to resubscribe after a disconnect+reconnect. Note that we are leaving out - * the watchman 'since' and 'relative_root' options, which are handled inside the + * or to resubscribe after a disconnect+reconnect. Note that we are leaving out + * the watchman 'since' and 'relative_root' options, which are handled inside the * WatchmanClient. */ WatchmanWatcher.prototype.createOptions = function() { let options = { - fields: ['name', 'exists', 'new'] + fields: ['name', 'exists', 'new'], }; // If the server has the wildmatch capability available it supports @@ -115,7 +115,7 @@ WatchmanWatcher.prototype.createOptions = function() { /** * Called by WatchmanClient when it receives an error from the watchman daemon. * - * @param {Object} error + * @param {Object} resp */ WatchmanWatcher.prototype.handleErrorEvent = function(error) { this.emit('error', error); @@ -149,8 +149,10 @@ WatchmanWatcher.prototype.handleFileChange = function(changeDescriptor) { relativePath = changeDescriptor.name; absPath = path.join(this.root, relativePath); - if (!(this._client.wildmatch && !this.hasIgnore) && - !common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath)) { + if ( + !(this._client.wildmatch && !this.hasIgnore) && + !common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath) + ) { return; } @@ -188,7 +190,12 @@ WatchmanWatcher.prototype.handleFileChange = function(changeDescriptor) { * @private */ -WatchmanWatcher.prototype.emitEvent = function(eventType, filepath, root, stat) { +WatchmanWatcher.prototype.emitEvent = function( + eventType, + filepath, + root, + stat +) { this.emit(eventType, filepath, root, stat); this.emit(ALL_EVENT, eventType, filepath, root, stat); }; From ffcb01a3d4f3cccbf8b0740aa30f404158cdc052 Mon Sep 17 00:00:00 2001 From: Dave Combs Date: Wed, 7 Feb 2018 14:05:00 -0800 Subject: [PATCH 3/4] fixed test failure - closing WatchmanWatcher instance was not passing the instance to close into WatchmanClient, resulted in spurious events --- src/watchman_watcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/watchman_watcher.js b/src/watchman_watcher.js index b315155..9440664 100644 --- a/src/watchman_watcher.js +++ b/src/watchman_watcher.js @@ -208,7 +208,7 @@ WatchmanWatcher.prototype.emitEvent = function( */ WatchmanWatcher.prototype.close = function(callback) { - this._client.closeWatcher(); + this._client.closeWatcher(this); callback && callback(null, true); }; From 60ce703133bf74126a5c38c6db80154007b0269e Mon Sep 17 00:00:00 2001 From: Dave Combs Date: Wed, 7 Feb 2018 16:51:49 -0800 Subject: [PATCH 4/4] adding SIGINT handler in WatchmanClient, to try to close out the watchman.Client if the process gets interrupted --- src/watchman_client.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/watchman_client.js b/src/watchman_client.js index 54e0012..231969e 100644 --- a/src/watchman_client.js +++ b/src/watchman_client.js @@ -43,6 +43,12 @@ function WatchmanClient(watchmanBinaryPath) { this._backoffTimes = this._setupBackoffTimes(); this._clientListeners = null; // direct listeners from here to watchman.Client. + + // Define a handler for if somehow the Node process gets interrupted. We need to + // close down the watchman.Client, if we have one. + process.on('SIGINT', () => { + this._clearLocalVars(); + }); } // Define 'wildmatch' property, which must be available when we call the