From 9743556fed2163886828630d552bd69a302e35c0 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Tue, 14 Feb 2017 09:36:11 -0800 Subject: [PATCH] Plugin API: Added module unpatching and updated tests correspondingly (#383) PR-URL: #383 --- src/trace-plugin-loader.js | 41 ++++++++++++------- test/test-plugin-loader.js | 82 ++++++++++++++++++-------------------- 2 files changed, 64 insertions(+), 59 deletions(-) diff --git a/src/trace-plugin-loader.js b/src/trace-plugin-loader.js index e50147e0d..8ee92ef7b 100644 --- a/src/trace-plugin-loader.js +++ b/src/trace-plugin-loader.js @@ -73,10 +73,7 @@ function activate(agent) { // hook into Module._load so that we can hook into userspace frameworks shimmer.wrap(Module, '_load', function(originalModuleLoad) { function loadAndPatch(instrumentation, moduleRoot, version) { - if (!instrumentation.patches[moduleRoot]) { - instrumentation.patches[moduleRoot] = {}; - } - var patchSet = instrumentation.patches[moduleRoot][version]; + var patchSet = instrumentation.patches[moduleRoot]; if (!patchSet) { // Load the plugin object var plugin = originalModuleLoad(instrumentation.file, module, false); @@ -87,17 +84,18 @@ function activate(agent) { patchSet[file] = { file: file, patch: patch.patch, + unpatch: patch.unpatch, intercept: patch.intercept }; } }); if (Object.keys(patchSet).length === 0) { - logger.warn(moduleRoot + ': version ' + version + ' not supported ' + + logger.warn(moduleRoot + ': version ' + version + ' not supported ' + 'by plugin.'); } - instrumentation.patches[moduleRoot][version] = patchSet; + instrumentation.patches[moduleRoot] = patchSet; } - Object.keys(patchSet).forEach(function(file) { + for (var file in patchSet) { var patch = patchSet[file]; if (!patch.module) { var loadPath = moduleRoot ? path.join(moduleRoot, patch.file) : patch.file; @@ -109,7 +107,7 @@ function activate(agent) { if (patch.intercept) { patch.module = patch.intercept(patch.module, api); } - }); + } var rootPatch = patchSet['']; if (rootPatch && rootPatch.intercept) { return rootPatch.module; @@ -119,14 +117,12 @@ function activate(agent) { } function moduleAlreadyPatched(instrumentation, moduleRoot, version) { - return instrumentation.patches[moduleRoot] && - instrumentation.patches[moduleRoot][version]; + return instrumentation.patches[moduleRoot]; } // Future requires get patched as they get loaded. return function Module_load(request, parent, isMain) { var instrumentation = plugins[request]; - if (instrumentation) { var moduleRoot = util.findModulePath(request, parent); var moduleVersion = util.findModuleVersion(moduleRoot, originalModuleLoad); @@ -140,17 +136,32 @@ function activate(agent) { return patchedRoot; } } - return originalModuleLoad.apply(this, arguments); }; }); } function deactivate() { - activated = false; + if (activated) { + activated = false; + for (var moduleName in plugins) { + var instrumentation = plugins[moduleName]; + for (var moduleRoot in instrumentation.patches) { + var patchSet = instrumentation.patches[moduleRoot]; + for (var file in patchSet) { + var patch = patchSet[file]; + if (patch.unpatch !== undefined) { + logger.info('Unpatching' + moduleName); + patch.unpatch(patch.module); + } + } + } + } + plugins = {}; - // unhook module.load - shimmer.unwrap(Module, '_load'); + // unhook module.load + shimmer.unwrap(Module, '_load'); + } } module.exports = { diff --git a/test/test-plugin-loader.js b/test/test-plugin-loader.js index 7477a3971..093995474 100644 --- a/test/test-plugin-loader.js +++ b/test/test-plugin-loader.js @@ -252,52 +252,12 @@ describe('Trace Plugin Loader', function() { 'Files internal to a module are patched'); }); - /** - * Alternately loads two versions of the same module, and checks that each one - * is patched differently. - */ - it('can patch multiple different versions of the same module', function() { - var v1 = { getVersion: function() { return '1.0.0'; } }; - var v2 = { getVersion: function() { return '2.0.0'; } }; - addModuleMock('module-h', '1.0.0', v1); - addModuleMock('module-h-plugin', '', [ - { - versions: '1.x', - patch: function(originalModule, api) { - shimmer.wrap(originalModule, 'getVersion', function(origGetVersion) { - return function() { - return origGetVersion() + ' is ok'; - }; - }); - } - }, - { - versions: '2.x', - patch: function(originalModule, api) { - shimmer.wrap(originalModule, 'getVersion', function(origGetVersion) { - return function() { - return origGetVersion() + ' is better'; - }; - }); - } - } - ]); - // Activate plugin loader - pluginLoader.activate(createFakeAgent({ - 'module-h': 'module-h-plugin' - })); - assert.strictEqual(require('module-h').getVersion(), '1.0.0 is ok', - 'Initial patch is correct'); - addModuleMock('module-h', '2.0.0', v2); - assert.strictEqual(require('module-h').getVersion(), '2.0.0 is better', - 'Second loaded version is also patched'); - addModuleMock('module-h', '1.0.0', v1); - assert.strictEqual(require('module-h').getVersion(), '1.0.0 is ok', - 'First loaded version doesn\'t get patched again'); - }); + // TODO(kjin): Add test to check that two modules with different versions + // can be loaded at the same time. Current test harness is insufficient for + // doing this. /** - * Uses module interception to + * Uses module interception to completely replace a module export */ it('can intercept modules', function() { addModuleMock('module-i', '1.0.0', function() { return 1; }); @@ -314,4 +274,38 @@ describe('Trace Plugin Loader', function() { assert.strictEqual(require('module-i')(), 2, 'Module can be intercepted'); }); + + /** + * Patches a module, then immediately unpatches it, then patches it again to + * show that patching isn't irreversible (and neither is unpatching) + */ + it('can unpatch', function() { + // Unfortunately, intercepted modules cannot be patched. + addModuleMock('module-j', '1.0.0', { + getPatchMode: function() { return 'none'; } + }); + addModuleMock('module-j-plugin', '', [{ + patch: function(originalModule, api) { + shimmer.wrap(originalModule, 'getPatchMode', function() { + return function() { return 'patch'; }; + }); + }, + unpatch: function(originalModule) { + shimmer.unwrap(originalModule, 'getPatchMode'); + } + }]); + assert.strictEqual(require('module-j').getPatchMode(), 'none'); + pluginLoader.activate(createFakeAgent({ + 'module-j': 'module-j-plugin' + })); + assert.strictEqual(require('module-j').getPatchMode(), 'patch'); + pluginLoader.deactivate(); + assert.strictEqual(require('module-j').getPatchMode(), 'none', + 'Module gets unpatched'); + pluginLoader.activate(createFakeAgent({ + 'module-j': 'module-j-plugin' + })); + assert.strictEqual(require('module-j').getPatchMode(), 'patch', + 'Patches still work after unpatching'); + }); });