From ae878b2050fb3d7f4fe28604856fea626791644e Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford Date: Tue, 31 Oct 2017 12:35:18 -0700 Subject: [PATCH] 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;