From e4381b4693dbed9746992c9a5b3be73b078a2171 Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford Date: Tue, 31 Oct 2017 11:28:56 -0700 Subject: [PATCH 1/4] Add an init watchdog for extension workers If an extension worker does not complete initialization in a reasonable amount of time (currently 15 seconds) then the worker will be terminated. A complete initialization includes registering an extension. --- src/dispatch/central-dispatch.js | 21 +++++- src/dispatch/shared-dispatch.js | 31 +++++++-- src/extension-support/extension-manager.js | 75 ++++++++++++++++++++-- 3 files changed, 117 insertions(+), 10 deletions(-) diff --git a/src/dispatch/central-dispatch.js b/src/dispatch/central-dispatch.js index 6dd236255..a60f9d97c 100644 --- a/src/dispatch/central-dispatch.js +++ b/src/dispatch/central-dispatch.js @@ -38,7 +38,7 @@ class CentralDispatch extends SharedDispatch { /** * Set a local object as the global provider of the specified service. * WARNING: Any method on the provider can be called from any worker within the dispatch system. - * @param {string} service - a globally unique string identifying this service. Examples: 'vm', 'gui', 'extension9'. + * @param {string} service - a globally unique string identifying this service. Examples: 'vm', 'gui'. * @param {object} provider - a local object which provides this service. * @returns {Promise} - a promise which will resolve once the service is registered. */ @@ -72,6 +72,25 @@ class CentralDispatch extends SharedDispatch { } } + /** + * Remove a service provider from the central dispatch service, un-registering any services it provides. + * If the provider is a worker its dispatch service will be shut down. + * The worker itself will NOT be terminated by this call. + * @param {Worker|object} provider - the worker or object instance to be removed. + */ + removeProvider (provider) { + const workerIndex = this.workers.indexOf(provider); + if (workerIndex !== -1) { + /** @TODO Should we ask the worker to shut down? If so, owner must wait before terminating the worker. */ + this.workers.splice(workerIndex, 1); + } + for (const serviceName in Object.keys(this.services)) { + if (this.services[serviceName] === provider) { + delete this.services[serviceName]; + } + } + } + /** * Fetch the service provider object for a particular service name. * @override diff --git a/src/dispatch/shared-dispatch.js b/src/dispatch/shared-dispatch.js index 1e4785bce..7c907d523 100644 --- a/src/dispatch/shared-dispatch.js +++ b/src/dispatch/shared-dispatch.js @@ -39,6 +39,24 @@ class SharedDispatch { * @type {int} */ this.nextResponseId = 0; + + /** + * While processing a call in _onMessage, this will be set to the originating object. + * At all other times it will be null. + * @type {object} + * @private + */ + this._callingWorker = null; + } + + /** + * Central dispatch: while processing a call from a worker, this will be set to the worker originating that call. + * Worker dispatch: while processing a call from the main thread, this will be set to the global object. + * At all other times it will be null. + * @returns {object} - the worker originating the current call, if any. + */ + get callingWorker () { + return this._callingWorker; } /** @@ -184,10 +202,15 @@ class SharedDispatch { message.args = message.args || []; let promise; if (message.service) { - if (message.service === 'dispatch') { - promise = this._onDispatchMessage(worker, message); - } else { - promise = this.call(message.service, message.method, ...message.args); + try { + this._callingWorker = worker; + if (message.service === 'dispatch') { + promise = this._onDispatchMessage(worker, message); + } else { + promise = this.call(message.service, message.method, ...message.args); + } + } finally { + this._callingWorker = null; } } else if (typeof message.responseId === 'undefined') { log.error(`Dispatch caught malformed message from a worker: ${JSON.stringify(event)}`); diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index d7c5fab05..31e915f8a 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -70,6 +70,14 @@ class ExtensionManager { */ this.pendingWorkers = []; + /** + * Set of workers currently being monitored by `_startWorkerWatchdog`. + * @see {_startWorkerWatchdog} + * @type {Set.} + * @private + */ + this._activeWatchdogs = new Set(); + /** * Keep a reference to the runtime so we can construct internal extension objects. * TODO: remove this in favor of extensions accessing the runtime as a service. @@ -99,7 +107,10 @@ class ExtensionManager { const ExtensionWorker = require('worker-loader?name=extension-worker.js!./extension-worker'); this.pendingExtensions.push({extensionURL, resolve, reject}); - dispatch.addWorker(new ExtensionWorker()); + + const worker = new ExtensionWorker(); + dispatch.addWorker(worker); + this._startWorkerWatchdog(worker); }); } @@ -110,16 +121,28 @@ class ExtensionManager { return [id, workerInfo.extensionURL]; } + /** + * Collect extension metadata from the specified service and begin the extension registration process. + * @param {string} serviceName - the name of the service hosting the extension. + */ registerExtensionService (serviceName) { dispatch.call(serviceName, 'getInfo').then(info => { this._registerExtensionInfo(serviceName, info); }); } + /** + * Called by an extension worker to indicate that the worker has finished initialization. + * @param {int} id - the worker ID. + * @param {*?} e - the error encountered during initialization, if any. + */ onWorkerInit (id, e) { const workerInfo = this.pendingWorkers[id]; delete this.pendingWorkers[id]; if (e) { + log.warn(`Extension manager forcibly terminating worker for extension with ID ${id}, URL ${ + workerInfo.extensionURL} due to error during initialization`); + workerInfo.worker.terminate(); workerInfo.reject(e); } else { workerInfo.resolve(id); @@ -133,16 +156,29 @@ class ExtensionManager { */ _registerInternalExtension (extensionObject) { const extensionInfo = extensionObject.getInfo(); - const serviceName = `extension.internal.${extensionInfo.id}`; + const fakeWorkerId = this.nextExtensionWorker++; + const serviceName = `extension.${fakeWorkerId}.${extensionInfo.id}`; return dispatch.setService(serviceName, extensionObject) .then(() => dispatch.call('extensions', 'registerExtensionService', serviceName)); } + /** + * Sanitize extension info then register its primitives with the VM. + * @param {string} serviceName - the name of the service hosting the extension + * @param {ExtensionInfo} extensionInfo - the extension's metadata + * @private + */ _registerExtensionInfo (serviceName, extensionInfo) { extensionInfo = this._prepareExtensionInfo(serviceName, extensionInfo); - dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).catch(e => { - log.error(`Failed to register primitives for extension on service ${serviceName}: ${JSON.stringify(e)}`); - }); + dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).then( + () => { + if (dispatch.callingWorker) { + this._stopWorkerWatchdog(dispatch.callingWorker); + } + }, + e => { + log.error(`Failed to register primitives for extension "${extensionInfo.id}": ${e.message}`); + }); } /** @@ -211,6 +247,35 @@ class ExtensionManager { } return blockInfo; } + + /** + * Start a watchdog to terminate this worker unless it registers an extension before the timeout. + * @param {object} worker - the worker to watch. + * @private + */ + _startWorkerWatchdog (worker) { + const timeout = 5 * 1000; // 5 seconds + + this._activeWatchdogs.add(worker); + setTimeout(() => { + if (this._activeWatchdogs.has(worker)) { + this._activeWatchdogs.delete(worker); + log.warn(`Worker watchdog timed out. Terminating worker.`); + dispatch.removeProvider(worker); + worker.terminate(); + } + }, timeout); + } + + /** + * Mark this worker as "safe" from the watchdog. + * @param {object} worker - the worker to mark as safe. + * @see {_startWorkerWatchdog} + * @private + */ + _stopWorkerWatchdog (worker) { + this._activeWatchdogs.delete(worker); + } } module.exports = ExtensionManager; From 670e51d33580e8a2e852b3b038bb3afc282f81b9 Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford Date: Tue, 31 Oct 2017 11:32:21 -0700 Subject: [PATCH 2/4] Refuse to register second extension with same ID If an extension attempts to register with the same ID as another extension which has already registered, the new registration is refused. If the extension is in a worker and no other extension is successfully registered by that worker, the watchdog system will terminate the "empty" worker. --- src/dispatch/central-dispatch.js | 13 +++++++ src/extension-support/extension-manager.js | 43 +++++++++++++++++----- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/dispatch/central-dispatch.js b/src/dispatch/central-dispatch.js index a60f9d97c..9b9681729 100644 --- a/src/dispatch/central-dispatch.js +++ b/src/dispatch/central-dispatch.js @@ -55,6 +55,19 @@ class CentralDispatch extends SharedDispatch { } } + /** + * Unregister a service without regard to which object or worker might provide it. + * @param {string} service - the globally unique string provided to `setService` when the service was registered. + * @see {setService} + */ + clearService (service) { + if (this.services.hasOwnProperty(service)) { + delete this.services[service]; + } else { + log.warn(`Central dispatch can't clear unknown service ${service}`); + } + } + /** * Add a worker to the message dispatch system. The worker must implement a compatible message dispatch framework. * The dispatcher will immediately attempt to "handshake" with the worker. diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index 31e915f8a..f671fa536 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -70,6 +70,13 @@ class ExtensionManager { */ this.pendingWorkers = []; + /** + * Set of loaded extension IDs. For built-in extensions the "URL" is the same as the ID; they differ in general. + * @type {Set.} + * @private + */ + this._loadedExtensions = new Set(); + /** * Set of workers currently being monitored by `_startWorkerWatchdog`. * @see {_startWorkerWatchdog} @@ -90,6 +97,17 @@ class ExtensionManager { }); } + /** + * Check whether an extension is registered or is in the process of loading. This is intended to control loading or + * adding extensions so it may return `true` before the extension is ready to be used. Use the promise returned by + * `loadExtensionURL` if you need to wait until the extension is truly ready. + * @param {string} extensionID - the ID (not URL) of the extension. + * @returns {boolean} - true if loaded, false otherwise. + */ + isExtensionLoaded (extensionID) { + return this._loadedExtensions.has(extensionID); + } + /** * Load an extension by URL or internal extension ID * @param {string} extensionURL - the URL for the extension to load OR the ID of an internal extension @@ -170,15 +188,22 @@ class ExtensionManager { */ _registerExtensionInfo (serviceName, extensionInfo) { extensionInfo = this._prepareExtensionInfo(serviceName, extensionInfo); - dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).then( - () => { - if (dispatch.callingWorker) { - this._stopWorkerWatchdog(dispatch.callingWorker); - } - }, - e => { - log.error(`Failed to register primitives for extension "${extensionInfo.id}": ${e.message}`); - }); + if (this.isExtensionLoaded(extensionInfo.id)) { + const message = `Ignoring attempt to load a second extension with ID ${extensionInfo.id}`; + log.warn(message); + dispatch.clearService(serviceName); + } else { + dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).then( + () => { + this._loadedExtensions.add(extensionInfo.id); + if (dispatch.callingWorker) { + this._stopWorkerWatchdog(dispatch.callingWorker); + } + }, + e => { + log.error(`Failed to register primitives for extension "${extensionInfo.id}": ${e.message}`); + }); + } } /** From ae878b2050fb3d7f4fe28604856fea626791644e Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford Date: Tue, 31 Oct 2017 12:35:18 -0700 Subject: [PATCH 3/4] Simplify extension duplication prevention station Since we will only have builtin extensions for a while, the more-complicated, watchdog-based extension duplication prevention has been replaced with one that treats the extension URL as equivalent to the extension ID. This is true only for built-in extensions, so once we start supporting third-party extensions we will likely need to bring back the watchdog (commit e4381b4693dbed9746992c9a5b3be73b078a2171) and the more-complex duplication prevention (commit 670e51d33580e8a2e852b3b038bb3afc282f81b9). The code here should be treated as a last resort: ideally the GUI should check if a particular extension is loaded (maybe by calling the Extension Manager's new `isExtensionLoaded` method) and avoid calling `loadExtensionURL` if it is. --- src/dispatch/central-dispatch.js | 34 +--------- src/dispatch/shared-dispatch.js | 31 ++------- src/extension-support/extension-manager.js | 75 ++++------------------ 3 files changed, 18 insertions(+), 122 deletions(-) diff --git a/src/dispatch/central-dispatch.js b/src/dispatch/central-dispatch.js index 9b9681729..6dd236255 100644 --- a/src/dispatch/central-dispatch.js +++ b/src/dispatch/central-dispatch.js @@ -38,7 +38,7 @@ class CentralDispatch extends SharedDispatch { /** * Set a local object as the global provider of the specified service. * WARNING: Any method on the provider can be called from any worker within the dispatch system. - * @param {string} service - a globally unique string identifying this service. Examples: 'vm', 'gui'. + * @param {string} service - a globally unique string identifying this service. Examples: 'vm', 'gui', 'extension9'. * @param {object} provider - a local object which provides this service. * @returns {Promise} - a promise which will resolve once the service is registered. */ @@ -55,19 +55,6 @@ class CentralDispatch extends SharedDispatch { } } - /** - * Unregister a service without regard to which object or worker might provide it. - * @param {string} service - the globally unique string provided to `setService` when the service was registered. - * @see {setService} - */ - clearService (service) { - if (this.services.hasOwnProperty(service)) { - delete this.services[service]; - } else { - log.warn(`Central dispatch can't clear unknown service ${service}`); - } - } - /** * Add a worker to the message dispatch system. The worker must implement a compatible message dispatch framework. * The dispatcher will immediately attempt to "handshake" with the worker. @@ -85,25 +72,6 @@ class CentralDispatch extends SharedDispatch { } } - /** - * Remove a service provider from the central dispatch service, un-registering any services it provides. - * If the provider is a worker its dispatch service will be shut down. - * The worker itself will NOT be terminated by this call. - * @param {Worker|object} provider - the worker or object instance to be removed. - */ - removeProvider (provider) { - const workerIndex = this.workers.indexOf(provider); - if (workerIndex !== -1) { - /** @TODO Should we ask the worker to shut down? If so, owner must wait before terminating the worker. */ - this.workers.splice(workerIndex, 1); - } - for (const serviceName in Object.keys(this.services)) { - if (this.services[serviceName] === provider) { - delete this.services[serviceName]; - } - } - } - /** * Fetch the service provider object for a particular service name. * @override diff --git a/src/dispatch/shared-dispatch.js b/src/dispatch/shared-dispatch.js index 7c907d523..1e4785bce 100644 --- a/src/dispatch/shared-dispatch.js +++ b/src/dispatch/shared-dispatch.js @@ -39,24 +39,6 @@ class SharedDispatch { * @type {int} */ this.nextResponseId = 0; - - /** - * While processing a call in _onMessage, this will be set to the originating object. - * At all other times it will be null. - * @type {object} - * @private - */ - this._callingWorker = null; - } - - /** - * Central dispatch: while processing a call from a worker, this will be set to the worker originating that call. - * Worker dispatch: while processing a call from the main thread, this will be set to the global object. - * At all other times it will be null. - * @returns {object} - the worker originating the current call, if any. - */ - get callingWorker () { - return this._callingWorker; } /** @@ -202,15 +184,10 @@ class SharedDispatch { message.args = message.args || []; let promise; if (message.service) { - try { - this._callingWorker = worker; - if (message.service === 'dispatch') { - promise = this._onDispatchMessage(worker, message); - } else { - promise = this.call(message.service, message.method, ...message.args); - } - } finally { - this._callingWorker = null; + if (message.service === 'dispatch') { + promise = this._onDispatchMessage(worker, message); + } else { + promise = this.call(message.service, message.method, ...message.args); } } else if (typeof message.responseId === 'undefined') { log.error(`Dispatch caught malformed message from a worker: ${JSON.stringify(event)}`); diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index f671fa536..d9dc3603e 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -71,20 +71,12 @@ class ExtensionManager { this.pendingWorkers = []; /** - * Set of loaded extension IDs. For built-in extensions the "URL" is the same as the ID; they differ in general. + * Set of loaded extension URLs/IDs (equivalent for built-in extensions). * @type {Set.} * @private */ this._loadedExtensions = new Set(); - /** - * Set of workers currently being monitored by `_startWorkerWatchdog`. - * @see {_startWorkerWatchdog} - * @type {Set.} - * @private - */ - this._activeWatchdogs = new Set(); - /** * Keep a reference to the runtime so we can construct internal extension objects. * TODO: remove this in favor of extensions accessing the runtime as a service. @@ -101,7 +93,7 @@ class ExtensionManager { * Check whether an extension is registered or is in the process of loading. This is intended to control loading or * adding extensions so it may return `true` before the extension is ready to be used. Use the promise returned by * `loadExtensionURL` if you need to wait until the extension is truly ready. - * @param {string} extensionID - the ID (not URL) of the extension. + * @param {string} extensionID - the ID of the extension. * @returns {boolean} - true if loaded, false otherwise. */ isExtensionLoaded (extensionID) { @@ -115,6 +107,13 @@ class ExtensionManager { */ loadExtensionURL (extensionURL) { if (builtinExtensions.hasOwnProperty(extensionURL)) { + /** @TODO dupe handling for non-builtin extensions. See commit 670e51d33580e8a2e852b3b038bb3afc282f81b9 */ + if (this.isExtensionLoaded(extensionURL)) { + const message = `Rejecting attempt to load a second extension with ID ${extensionURL}`; + log.warn(message); + return Promise.reject(new Error(message)); + } + const extension = builtinExtensions[extensionURL]; const extensionInstance = new extension(this.runtime); return this._registerInternalExtension(extensionInstance); @@ -125,10 +124,7 @@ class ExtensionManager { const ExtensionWorker = require('worker-loader?name=extension-worker.js!./extension-worker'); this.pendingExtensions.push({extensionURL, resolve, reject}); - - const worker = new ExtensionWorker(); - dispatch.addWorker(worker); - this._startWorkerWatchdog(worker); + dispatch.addWorker(new ExtensionWorker()); }); } @@ -158,9 +154,6 @@ class ExtensionManager { const workerInfo = this.pendingWorkers[id]; delete this.pendingWorkers[id]; if (e) { - log.warn(`Extension manager forcibly terminating worker for extension with ID ${id}, URL ${ - workerInfo.extensionURL} due to error during initialization`); - workerInfo.worker.terminate(); workerInfo.reject(e); } else { workerInfo.resolve(id); @@ -188,22 +181,9 @@ class ExtensionManager { */ _registerExtensionInfo (serviceName, extensionInfo) { extensionInfo = this._prepareExtensionInfo(serviceName, extensionInfo); - if (this.isExtensionLoaded(extensionInfo.id)) { - const message = `Ignoring attempt to load a second extension with ID ${extensionInfo.id}`; - log.warn(message); - dispatch.clearService(serviceName); - } else { - dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).then( - () => { - this._loadedExtensions.add(extensionInfo.id); - if (dispatch.callingWorker) { - this._stopWorkerWatchdog(dispatch.callingWorker); - } - }, - e => { - log.error(`Failed to register primitives for extension "${extensionInfo.id}": ${e.message}`); - }); - } + dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).catch(e => { + log.error(`Failed to register primitives for extension on service ${serviceName}: ${JSON.stringify(e)}`); + }); } /** @@ -272,35 +252,6 @@ class ExtensionManager { } return blockInfo; } - - /** - * Start a watchdog to terminate this worker unless it registers an extension before the timeout. - * @param {object} worker - the worker to watch. - * @private - */ - _startWorkerWatchdog (worker) { - const timeout = 5 * 1000; // 5 seconds - - this._activeWatchdogs.add(worker); - setTimeout(() => { - if (this._activeWatchdogs.has(worker)) { - this._activeWatchdogs.delete(worker); - log.warn(`Worker watchdog timed out. Terminating worker.`); - dispatch.removeProvider(worker); - worker.terminate(); - } - }, timeout); - } - - /** - * Mark this worker as "safe" from the watchdog. - * @param {object} worker - the worker to mark as safe. - * @see {_startWorkerWatchdog} - * @private - */ - _stopWorkerWatchdog (worker) { - this._activeWatchdogs.delete(worker); - } } module.exports = ExtensionManager; From 8bb48e2a53c80efd922911ae770dad09ca8de3d8 Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford Date: Tue, 31 Oct 2017 13:00:19 -0700 Subject: [PATCH 4/4] Add built-in extension "URL" to _loadedExtensions Dupe prevention works better when you actually record the necessary info... --- src/extension-support/extension-manager.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index d9dc3603e..ef3d56aa8 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -116,7 +116,9 @@ class ExtensionManager { const extension = builtinExtensions[extensionURL]; const extensionInstance = new extension(this.runtime); - return this._registerInternalExtension(extensionInstance); + return this._registerInternalExtension(extensionInstance).then(() => { + this._loadedExtensions.add(extensionURL); + }); } return new Promise((resolve, reject) => {