diff --git a/doc/api/hooks_overview.md b/doc/api/hooks_overview.md index 1de547c90..35a88dbe1 100644 --- a/doc/api/hooks_overview.md +++ b/doc/api/hooks_overview.md @@ -4,48 +4,114 @@ A hook function is registered with a hook via the plugin's `ep.json` file. See the Plugins section for details. A hook may have many registered functions from different plugins. -When a hook is invoked, its registered functions are called with three -arguments: +Some hooks call their registered functions one at a time until one of them +returns a value. Others always call all of their registered functions and +combine the results (if applicable). -1. hookName - The name of the hook being invoked. -2. context - An object with some relevant information about the context of the +## Registered hook functions + +Note: The documentation in this section applies to every hook unless the +hook-specific documentation says otherwise. + +### Arguments + +Hook functions are called with three arguments: + +1. `hookName` - The name of the hook being invoked. +2. `context` - An object with some relevant information about the context of the call. See the hook-specific documentation for details. -3. callback - Function to call when done. This callback takes a single argument, - the meaning of which depends on the hook. See the "Return values" section for - general information that applies to most hooks. The value returned by this - callback must be returned by the hook function unless otherwise specified. +3. `cb` - For asynchronous operations this callback can be called to signal + completion and optionally provide a return value. The callback takes a single + argument, the meaning of which depends on the hook (see the "Return values" + section for general information that applies to most hooks). This callback + always returns `undefined`. -## Return values +### Expected behavior -Note: This section applies to every hook unless the hook-specific documentation -says otherwise. +The presence of a callback parameter suggests that every hook function can run +asynchronously. While that is the eventual goal, there are some legacy hooks +that expect their hook functions to provide a value synchronously. For such +hooks, the hook functions must do one of the following: -Hook functions return zero or more values to Etherpad by passing an array to the -provided callback. Hook functions typically provide a single value (array of -length one). If the function does not want to or need to provide a value, it may -pass an empty array or `undefined` (which is treated the same as an empty -array). Hook functions may also provide more than one value (array of length two -or more). +* Call the callback with a non-Promise value (`undefined` is acceptable) and + return `undefined`, in that order. +* Return a non-Promise value other than `undefined` (`null` is acceptable) and + never call the callback. Note that `async` functions *always* return a + Promise, so they must never be used for synchronous hooks. +* Only have two parameters (`hookName` and `context`) and return any non-Promise + value (`undefined` is acceptable). -Some hooks concatenate the arrays provided by its registered functions. For -example, if a hook's registered functions pass `[1, 2]`, `undefined`, `[3, 4]`, -`[]`, and `[5]` to the provided callback, then the hook's return value is `[1, -2, 3, 4, 5]`. +For hooks that permit asynchronous behavior, the hook functions must do one or +more of the following: -Other hooks only use the first non-empty array provided by a registered -function. In this case, each of the hook's registered functions is called one at -a time until one provides a non-empty array. The remaining functions are -skipped. If none of the functions provide a non-empty array, or there are no -registered functions, the hook's return value is `[]`. +* Return `undefined` and call the callback, in either order. +* Return something other than `undefined` (`null` is acceptable) and never call + the callback. Note that `async` functions *always* return a Promise, so they + must never call the callback. +* Only have two parameters (`hookName` and `context`). -Example: +Note that the acceptable behaviors for asynchronous hook functions is a superset +of the acceptable behaviors for synchronous hook functions. -``` -exports.abstractHook = (hookName, context, callback) => { - if (notApplicableToThisPlugin(context)) { - return callback(); - } - const value = doSomeProcessing(context); - return callback([value]); +WARNING: The number of parameters is determined by examining +[Function.length](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length), +which does not count [default +parameters](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters) +or ["rest" +parameters](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters). +To avoid problems, do not use default or rest parameters when defining hook +functions. + +### Return values + +A hook function can provide a value to Etherpad in one of the following ways: + +* Pass the desired value as the first argument to the callback. +* Return the desired value directly. The value must not be `undefined` unless + the hook function only has two parameters. (Hook functions with three + parameters that want to provide `undefined` should instead use the callback.) +* For hooks that permit asynchronous behavior, return a Promise that resolves to + the desired value. +* For hooks that permit asynchronous behavior, pass a Promise that resolves to + the desired value as the first argument to the callback. + +Examples: + +```javascript +exports.exampleOne = (hookName, context, callback) => { + return 'valueOne'; +}; + +exports.exampleTwo = (hookName, context, callback) => { + callback('valueTwo'); + return; +}; + +// ONLY FOR HOOKS THAT PERMIT ASYNCHRONOUS BEHAVIOR +exports.exampleThree = (hookName, context, callback) => { + return new Promise('valueThree'); +}; + +// ONLY FOR HOOKS THAT PERMIT ASYNCHRONOUS BEHAVIOR +exports.exampleFour = (hookName, context, callback) => { + callback(new Promise('valueFour')); + return; +}; + +// ONLY FOR HOOKS THAT PERMIT ASYNCHRONOUS BEHAVIOR +exports.exampleFive = async (hookName, context) => { + // Note that this function is async, so it actually returns a Promise that + // is resolved to 'valueFive'. + return 'valueFive'; }; ``` + +Etherpad collects the values provided by the hook functions into an array, +filters out all `undefined` values, then flattens the array one level. +Flattening one level makes it possible for a hook function to behave as if it +were multiple separate hook functions. + +For example: Suppose a hook has eight registered functions that return the +following values: `1`, `[2]`, `['3a', '3b']` `[[4]]`, `undefined`, +`[undefined]`, `[]`, and `null`. The value returned to the caller of the hook is +`[1, 2, '3a', '3b', [4], undefined, null]`. diff --git a/src/package-lock.json b/src/package-lock.json index 60ec0887f..5423e36e2 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -454,6 +454,51 @@ "resolved": "https://registry.npmjs.org/@kwsites/promise-deferred/-/promise-deferred-1.1.1.tgz", "integrity": "sha512-GaHYm+c0O9MjZRu0ongGBRbinu8gVAMd2UZjji6jVmqKtZluZnptXGWhz1E8j8D2HJ3f/yMxKAUC0b+57wncIw==" }, + "@sinonjs/commons": { + "version": "1.8.1", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-1.8.1.tgz", + "integrity": "sha512-892K+kWUUi3cl+LlqEWIDrhvLgdL79tECi8JZUyq6IviKy/DNhuzCRlbHUjxK89f4ypPMMaFnFuR9Ie6DoIMsw==", + "dev": true, + "requires": { + "type-detect": "4.0.8" + } + }, + "@sinonjs/fake-timers": { + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-6.0.1.tgz", + "integrity": "sha512-MZPUxrmFubI36XS1DI3qmI0YdN1gks62JtFZvxR67ljjSNCeK6U08Zx4msEWOXuofgqUt6zPHSi1H9fbjR/NRA==", + "dev": true, + "requires": { + "@sinonjs/commons": "^1.7.0" + } + }, + "@sinonjs/formatio": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/@sinonjs/formatio/-/formatio-5.0.1.tgz", + "integrity": "sha512-KaiQ5pBf1MpS09MuA0kp6KBQt2JUOQycqVG1NZXvzeaXe5LGFqAKueIS0bw4w0P9r7KuBSVdUk5QjXsUdu2CxQ==", + "dev": true, + "requires": { + "@sinonjs/commons": "^1", + "@sinonjs/samsam": "^5.0.2" + } + }, + "@sinonjs/samsam": { + "version": "5.2.0", + "resolved": "https://registry.npmjs.org/@sinonjs/samsam/-/samsam-5.2.0.tgz", + "integrity": "sha512-CaIcyX5cDsjcW/ab7HposFWzV1kC++4HNsfnEdFJa7cP1QIuILAKV+BgfeqRXhcnSAc76r/Rh/O5C+300BwUIw==", + "dev": true, + "requires": { + "@sinonjs/commons": "^1.6.0", + "lodash.get": "^4.4.2", + "type-detect": "^4.0.8" + } + }, + "@sinonjs/text-encoding": { + "version": "0.7.1", + "resolved": "https://registry.npmjs.org/@sinonjs/text-encoding/-/text-encoding-0.7.1.tgz", + "integrity": "sha512-+iTbntw2IZPb/anVDbypzfQa+ay64MW0Zo8aJ8gZPWMMK6/OubMVb6lUPMagqjOPnmtauXnFCACVl3O7ogjeqQ==", + "dev": true + }, "@types/caseless": { "version": "0.12.2", "resolved": "https://registry.npmjs.org/@types/caseless/-/caseless-0.12.2.tgz", @@ -2742,6 +2787,12 @@ "verror": "1.10.0" } }, + "just-extend": { + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/just-extend/-/just-extend-4.1.1.tgz", + "integrity": "sha512-aWgeGFW67BP3e5181Ep1Fv2v8z//iBJfrvyTnq8wG86vEESwmonn1zPBJ0VfmT9CJq2FIT0VsETtrNFm2a+SHA==", + "dev": true + }, "jwa": { "version": "1.4.1", "resolved": "https://registry.npmjs.org/jwa/-/jwa-1.4.1.tgz", @@ -3269,6 +3320,30 @@ "resolved": "https://registry.npmjs.org/negotiator/-/negotiator-0.6.2.tgz", "integrity": "sha512-hZXc7K2e+PgeI1eDBe/10Ard4ekbfrrqG8Ep+8Jmf4JID2bNg7NvCPOZN+kfF574pFQI7mum2AUqDidoKqcTOw==" }, + "nise": { + "version": "4.0.4", + "resolved": "https://registry.npmjs.org/nise/-/nise-4.0.4.tgz", + "integrity": "sha512-bTTRUNlemx6deJa+ZyoCUTRvH3liK5+N6VQZ4NIw90AgDXY6iPnsqplNFf6STcj+ePk0H/xqxnP75Lr0J0Fq3A==", + "dev": true, + "requires": { + "@sinonjs/commons": "^1.7.0", + "@sinonjs/fake-timers": "^6.0.0", + "@sinonjs/text-encoding": "^0.7.1", + "just-extend": "^4.0.2", + "path-to-regexp": "^1.7.0" + }, + "dependencies": { + "path-to-regexp": { + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-1.8.0.tgz", + "integrity": "sha512-n43JRhlUKUAlibEJhPeir1ncUID16QnEjNpwzNdO3Lm4ywrBpBZ5oLD0I6br9evr1Y9JTqwRtAh7JLoOzAQdVA==", + "dev": true, + "requires": { + "isarray": "0.0.1" + } + } + } + }, "node-environment-flags": { "version": "1.0.6", "resolved": "https://registry.npmjs.org/node-environment-flags/-/node-environment-flags-1.0.6.tgz", @@ -7396,6 +7471,44 @@ } } }, + "sinon": { + "version": "9.2.0", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-9.2.0.tgz", + "integrity": "sha512-eSNXz1XMcGEMHw08NJXSyTHIu6qTCOiN8x9ODACmZpNQpr0aXTBXBnI4xTzQzR+TEpOmLiKowGf9flCuKIzsbw==", + "dev": true, + "requires": { + "@sinonjs/commons": "^1.8.1", + "@sinonjs/fake-timers": "^6.0.1", + "@sinonjs/formatio": "^5.0.1", + "@sinonjs/samsam": "^5.2.0", + "diff": "^4.0.2", + "nise": "^4.0.4", + "supports-color": "^7.1.0" + }, + "dependencies": { + "diff": { + "version": "4.0.2", + "resolved": "https://registry.npmjs.org/diff/-/diff-4.0.2.tgz", + "integrity": "sha512-58lmxKSA4BNyLz+HHMUzlOEpg09FV+ev6ZMe3vJihgdxzgcwZ8VoEEPmALCZG9LmqfVoNMMKpttIYTVG6uDY7A==", + "dev": true + }, + "has-flag": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/has-flag/-/has-flag-4.0.0.tgz", + "integrity": "sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ==", + "dev": true + }, + "supports-color": { + "version": "7.2.0", + "resolved": "https://registry.npmjs.org/supports-color/-/supports-color-7.2.0.tgz", + "integrity": "sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==", + "dev": true, + "requires": { + "has-flag": "^4.0.0" + } + } + } + }, "slide": { "version": "1.1.6", "resolved": "https://registry.npmjs.org/slide/-/slide-1.1.6.tgz", diff --git a/src/package.json b/src/package.json index 1d57e6116..44750a9b5 100644 --- a/src/package.json +++ b/src/package.json @@ -77,14 +77,15 @@ "etherpad-lite": "./node/server.js" }, "devDependencies": { + "etherpad-cli-client": "0.0.9", "mocha": "7.1.2", "mocha-froth": "^0.2.10", "nyc": "15.0.1", "set-cookie-parser": "^2.4.6", + "sinon": "^9.2.0", "superagent": "^3.8.3", "supertest": "4.0.2", - "wd": "1.12.1", - "etherpad-cli-client": "0.0.9" + "wd": "1.12.1" }, "engines": { "node": ">=10.13.0", diff --git a/src/static/js/pluginfw/hooks.js b/src/static/js/pluginfw/hooks.js index 3032db36f..c4719b18e 100644 --- a/src/static/js/pluginfw/hooks.js +++ b/src/static/js/pluginfw/hooks.js @@ -1,3 +1,5 @@ +/* global exports, require */ + var _ = require("underscore"); var pluginDefs = require('./plugin_defs'); @@ -17,8 +19,8 @@ function checkDeprecation(hook) { const notice = exports.deprecationNotices[hook.hook_name]; if (notice == null) return; if (deprecationWarned[hook.hook_fn_name]) return; - console.warn('%s hook used by the %s plugin (%s) is deprecated: %s', - hook.hook_name, hook.part.name, hook.hook_fn_name, notice); + console.warn(`${hook.hook_name} hook used by the ${hook.part.name} plugin ` + + `(${hook.hook_fn_name}) is deprecated: ${notice}`); deprecationWarned[hook.hook_fn_name] = true; } @@ -76,49 +78,287 @@ exports.mapFirst = function (lst, fn, cb, predicate) { next(); } -exports.callAll = function (hook_name, args) { - if (!args) args = {}; - if (pluginDefs.hooks[hook_name] === undefined) return []; - return _.flatten(_.map(pluginDefs.hooks[hook_name], function(hook) { - return hookCallWrapper(hook, hook_name, args); - }), true); -} +// Calls the hook function synchronously and returns the value provided by the hook function (via +// callback or return value). +// +// A synchronous hook function can provide a value in these ways: +// +// * Call the callback, passing the desired value (which may be `undefined`) directly as the first +// argument, then return `undefined`. +// * For hook functions with three (or more) parameters: Directly return the desired value, which +// must not be `undefined`. Note: If a three-parameter hook function directly returns +// `undefined` and it has not already called the callback then it is indicating that it is not +// yet done and will eventually call the callback. This behavior is not supported by synchronous +// hooks. +// * For hook functions with two (or fewer) parameters: Directly return the desired value (which +// may be `undefined`). +// +// The callback passed to a hook function is guaranteed to return `undefined`, so it is safe for +// hook functions to do `return cb(value);`. +// +// A hook function can signal an error by throwing. +// +// A hook function settles when it provides a value (via callback or return) or throws. If a hook +// function attempts to settle again (e.g., call the callback again, or call the callback and also +// return a value) then the second attempt has no effect except either an error message is logged or +// there will be an unhandled promise rejection depending on whether the the subsequent attempt is a +// duplicate (same value or error) or different, respectively. +// +// See the tests in tests/backend/specs/hooks.js for examples of supported and prohibited behaviors. +// +function callHookFnSync(hook, context) { + checkDeprecation(hook); -async function aCallAll(hook_name, args, cb) { - if (!args) args = {}; - if (!cb) cb = function () {}; - if (pluginDefs.hooks[hook_name] === undefined) return cb(null, []); + // This var is used to keep track of whether the hook function already settled. + let outcome; - var hooksPromises = pluginDefs.hooks[hook_name].map(async function(hook, index) { - return await hookCallWrapper(hook, hook_name, args, function (res) { - return Promise.resolve(res); - }); - }); + // This is used to prevent recursion. + let doubleSettleErr; - var result = await Promise.all(hooksPromises); - - // after forEach - cb(null, _.flatten(result, true)); -} - -/* return a Promise if cb is not supplied */ -exports.aCallAll = function (hook_name, args, cb) { - if (cb === undefined) { - try{ - return new Promise(function(resolve, reject) { - aCallAll(hook_name, args, function(err, res) { - return err ? reject(err) : resolve(res); - }); - }); - }catch(e){ - $.gritter.removeAll(); - $.gritter.add("Please update your web browser") + const settle = (err, val, how) => { + doubleSettleErr = null; + const state = err == null ? 'resolved' : 'rejected'; + if (outcome != null) { + // It was already settled, which indicates a bug. + const action = err == null ? 'resolve' : 'reject'; + const msg = (`DOUBLE SETTLE BUG IN HOOK FUNCTION (plugin: ${hook.part.name}, ` + + `function name: ${hook.hook_fn_name}, hook: ${hook.hook_name}): ` + + `Attempt to ${action} via ${how} but it already ${outcome.state} ` + + `via ${outcome.how}. Ignoring this attempt to ${action}.`); + console.error(msg); + if (state !== outcome.state || (err == null ? val !== outcome.val : err !== outcome.err)) { + // The second settle attempt differs from the first, which might indicate a serious bug. + doubleSettleErr = new Error(msg); + throw doubleSettleErr; + } + return; } - } else { - return aCallAll(hook_name, args, cb); + outcome = {state, err, val, how}; + if (val && typeof val.then === 'function') { + console.error(`PROHIBITED PROMISE BUG IN HOOK FUNCTION (plugin: ${hook.part.name}, ` + + `function name: ${hook.hook_fn_name}, hook: ${hook.hook_name}): ` + + 'The hook function provided a "thenable" (e.g., a Promise) which is ' + + 'prohibited because the hook expects to get the value synchronously.'); + } + }; + + // IMPORTANT: This callback must return `undefined` so that a hook function can safely do + // `return callback(value);` for backwards compatibility. + const callback = (ret) => { + settle(null, ret, 'callback'); + }; + + let val; + try { + val = hook.hook_fn(hook.hook_name, context, callback); + } catch (err) { + if (err === doubleSettleErr) throw err; // Avoid recursion. + try { + settle(err, null, 'thrown exception'); + } catch (doubleSettleErr) { + // Schedule the throw of the double settle error on the event loop via + // Promise.resolve().then() (which will result in an unhandled Promise rejection) so that the + // original error is the error that is seen by the caller. Fixing the original error will + // likely fix the double settle bug, so the original error should get priority. + Promise.resolve().then(() => { throw doubleSettleErr; }); + } + throw err; } + + // IMPORTANT: This MUST check for undefined -- not nullish -- because some hooks intentionally use + // null as a special value. + if (val === undefined) { + if (outcome != null) return outcome.val; // Already settled via callback. + if (hook.hook_fn.length >= 3) { + console.error(`UNSETTLED FUNCTION BUG IN HOOK FUNCTION (plugin: ${hook.part.name}, ` + + `function name: ${hook.hook_fn_name}, hook: ${hook.hook_name}): ` + + 'The hook function neither called the callback nor returned a non-undefined ' + + 'value. This is prohibited because it will result in freezes when a future ' + + 'version of Etherpad updates the hook to support asynchronous behavior.'); + } else { + // The hook function is assumed to not have a callback parameter, so fall through and accept + // `undefined` as the resolved value. + // + // IMPORTANT: "Rest" parameters and default parameters are not counted in`Function.length`, so + // the assumption does not hold for wrappers like `(...args) => { real(...args); }`. Such + // functions will still work properly without any logged warnings or errors for now, but: + // * Once the hook is upgraded to support asynchronous hook functions, calling the callback + // will (eventually) cause a double settle error, and the function might prematurely + // resolve to `undefined` instead of the desired value. + // * The above "unsettled function" warning is not logged if the function fails to call the + // callback like it is supposed to. + } + } + + settle(null, val, 'returned value'); + return outcome.val; } +// Invokes all registered hook functions synchronously. +// +// Arguments: +// * hookName: Name of the hook to invoke. +// * context: Passed unmodified to the hook functions, except nullish becomes {}. +// +// Return value: +// A flattened array of hook results. Specifically, it is equivalent to doing the following: +// 1. Collect all values returned by the hook functions into an array. +// 2. Convert each `undefined` entry into `[]`. +// 3. Flatten one level. +exports.callAll = function (hookName, context) { + if (context == null) context = {}; + const hooks = pluginDefs.hooks[hookName] || []; + return _.flatten(hooks.map((hook) => { + const ret = callHookFnSync(hook, context); + // `undefined` (but not `null`!) is treated the same as []. + if (ret === undefined) return []; + return ret; + }), 1); +}; + +// Calls the hook function asynchronously and returns a Promise that either resolves to the hook +// function's provided value or rejects with an error generated by the hook function. +// +// An asynchronous hook function can provide a value in these ways: +// +// * Call the callback, passing a Promise (or thenable) that resolves to the desired value (which +// may be `undefined`) as the first argument. +// * Call the callback, passing the desired value (which may be `undefined`) directly as the first +// argument. +// * Return a Promise (or thenable) that resolves to the desired value (which may be `undefined`). +// * For hook functions with three (or more) parameters: Directly return the desired value, which +// must not be `undefined`. Note: If a hook function directly returns `undefined` and it has not +// already called the callback then it is indicating that it is not yet done and will eventually +// call the callback. +// * For hook functions with two (or fewer) parameters: Directly return the desired value (which +// may be `undefined`). +// +// The callback passed to a hook function is guaranteed to return `undefined`, so it is safe for +// hook functions to do `return cb(valueOrPromise);`. +// +// A hook function can signal an error in these ways: +// +// * Throw. +// * Return a Promise that rejects. +// * Pass a Promise that rejects as the first argument to the provided callback. +// +// A hook function settles when it directly provides a value, when it throws, or when the Promise it +// provides settles (resolves or rejects). If a hook function attempts to settle again (e.g., call +// the callback again, or return a value and also call the callback) then the second attempt has no +// effect except either an error message is logged or an Error object is thrown depending on whether +// the the subsequent attempt is a duplicate (same value or error) or different, respectively. +// +// See the tests in tests/backend/specs/hooks.js for examples of supported and prohibited behaviors. +// +async function callHookFnAsync(hook, context) { + checkDeprecation(hook); + return await new Promise((resolve, reject) => { + // This var is used to keep track of whether the hook function already settled. + let outcome; + + const settle = (err, val, how) => { + const state = err == null ? 'resolved' : 'rejected'; + if (outcome != null) { + // It was already settled, which indicates a bug. + const action = err == null ? 'resolve' : 'reject'; + const msg = (`DOUBLE SETTLE BUG IN HOOK FUNCTION (plugin: ${hook.part.name}, ` + + `function name: ${hook.hook_fn_name}, hook: ${hook.hook_name}): ` + + `Attempt to ${action} via ${how} but it already ${outcome.state} ` + + `via ${outcome.how}. Ignoring this attempt to ${action}.`); + console.error(msg); + if (state !== outcome.state || (err == null ? val !== outcome.val : err !== outcome.err)) { + // The second settle attempt differs from the first, which might indicate a serious bug. + throw new Error(msg); + } + return; + } + outcome = {state, err, val, how}; + if (err == null) { resolve(val); } else { reject(err); } + }; + + // IMPORTANT: This callback must return `undefined` so that a hook function can safely do + // `return callback(value);` for backwards compatibility. + const callback = (ret) => { + // Wrap ret in a Promise so that a hook function can do `callback(asyncFunction());`. Note: If + // ret is a Promise (or other thenable), Promise.resolve() will flatten it into this new + // Promise. + Promise.resolve(ret).then( + (val) => settle(null, val, 'callback'), + (err) => settle(err, null, 'rejected Promise passed to callback')); + }; + + let ret; + try { + ret = hook.hook_fn(hook.hook_name, context, callback); + } catch (err) { + try { + settle(err, null, 'thrown exception'); + } catch (doubleSettleErr) { + // Schedule the throw of the double settle error on the event loop via + // Promise.resolve().then() (which will result in an unhandled Promise rejection) so that + // the original error is the error that is seen by the caller. Fixing the original error + // will likely fix the double settle bug, so the original error should get priority. + Promise.resolve().then(() => { throw doubleSettleErr; }); + } + throw err; + } + + // IMPORTANT: This MUST check for undefined -- not nullish -- because some hooks intentionally + // use null as a special value. + if (ret === undefined) { + if (hook.hook_fn.length >= 3) { + // The hook function has a callback parameter and it returned undefined, which means the + // hook function will settle (or has already settled) via the provided callback. + return; + } else { + // The hook function is assumed to not have a callback parameter, so fall through and accept + // `undefined` as the resolved value. + // + // IMPORTANT: "Rest" parameters and default parameters are not counted in `Function.length`, + // so the assumption does not hold for wrappers like `(...args) => { real(...args); }`. For + // such functions, calling the callback will (eventually) cause a double settle error, and + // the function might prematurely resolve to `undefined` instead of the desired value. + } + } + + // Wrap ret in a Promise so that hook functions can be async (or otherwise return a Promise). + // Note: If ret is a Promise (or other thenable), Promise.resolve() will flatten it into this + // new Promise. + Promise.resolve(ret).then( + (val) => settle(null, val, 'returned value'), + (err) => settle(err, null, 'Promise rejection')); + }); +} + +// Invokes all registered hook functions asynchronously. +// +// Arguments: +// * hookName: Name of the hook to invoke. +// * context: Passed unmodified to the hook functions, except nullish becomes {}. +// * cb: Deprecated callback. The following: +// const p1 = hooks.aCallAll('myHook', context, cb); +// is equivalent to: +// const p2 = hooks.aCallAll('myHook', context).then((val) => cb(null, val), cb); +// +// Return value: +// If cb is nullish, this function resolves to a flattened array of hook results. Specifically, it +// is equivalent to doing the following: +// 1. Collect all values returned by the hook functions into an array. +// 2. Convert each `undefined` entry into `[]`. +// 3. Flatten one level. +// If cb is non-null, this function resolves to the value returned by cb. +exports.aCallAll = async (hookName, context, cb) => { + if (context == null) context = {}; + const hooks = pluginDefs.hooks[hookName] || []; + let resultsPromise = Promise.all(hooks.map((hook) => { + return callHookFnAsync(hook, context) + // `undefined` (but not `null`!) is treated the same as []. + .then((result) => (result === undefined) ? [] : result); + })).then((results) => _.flatten(results, 1)); + if (cb != null) resultsPromise = resultsPromise.then((val) => cb(null, val), cb); + return await resultsPromise; +}; + exports.callFirst = function (hook_name, args) { if (!args) args = {}; if (pluginDefs.hooks[hook_name] === undefined) return []; @@ -165,3 +405,9 @@ exports.callAllStr = function(hook_name, args, sep, pre, post) { } return newCallhooks.join(sep || ""); } + +exports.exportedForTestingOnly = { + callHookFnAsync, + callHookFnSync, + deprecationWarned, +}; diff --git a/tests/backend/common.js b/tests/backend/common.js index 6c780219c..8445c2e1f 100644 --- a/tests/backend/common.js +++ b/tests/backend/common.js @@ -1,7 +1,10 @@ +/* global __dirname, exports, require */ + function m(mod) { return __dirname + '/../../src/' + mod; } const apiHandler = require(m('node/handler/APIHandler')); const log4js = require(m('node_modules/log4js')); +const process = require('process'); const server = require(m('node/server')); const settings = require(m('node/utils/Settings')); const supertest = require(m('node_modules/supertest')); @@ -18,6 +21,10 @@ exports.logger = log4js.getLogger('test'); const logLevel = exports.logger.level; +// Mocha doesn't monitor unhandled Promise rejections, so convert them to uncaught exceptions. +// https://github.com/mochajs/mocha/issues/2640 +process.on('unhandledRejection', (reason, promise) => { throw reason; }); + exports.init = async function() { if (inited) return exports.agent; inited = true; diff --git a/tests/backend/specs/hooks.js b/tests/backend/specs/hooks.js new file mode 100644 index 000000000..3b2c15e7f --- /dev/null +++ b/tests/backend/specs/hooks.js @@ -0,0 +1,893 @@ +/* global __dirname, __filename, afterEach, beforeEach, describe, it, process, require */ + +function m(mod) { return __dirname + '/../../../src/' + mod; } + +const assert = require('assert').strict; +const common = require('../common'); +const hooks = require(m('static/js/pluginfw/hooks')); +const plugins = require(m('static/js/pluginfw/plugin_defs')); +const sinon = require(m('node_modules/sinon')); + +const logger = common.logger; + +describe(__filename, function() { + const hookName = 'testHook'; + const hookFnName = 'testPluginFileName:testHookFunctionName'; + let testHooks; // Convenience shorthand for plugins.hooks[hookName]. + let hook; // Convenience shorthand for plugins.hooks[hookName][0]. + + beforeEach(async function() { + // Make sure these are not already set so that we don't accidentally step on someone else's + // toes: + assert(plugins.hooks[hookName] == null); + assert(hooks.deprecationNotices[hookName] == null); + assert(hooks.exportedForTestingOnly.deprecationWarned[hookFnName] == null); + + // Many of the tests only need a single registered hook function. Set that up here to reduce + // boilerplate. + hook = makeHook(); + plugins.hooks[hookName] = [hook]; + testHooks = plugins.hooks[hookName]; + }); + + afterEach(async function() { + sinon.restore(); + delete plugins.hooks[hookName]; + delete hooks.deprecationNotices[hookName]; + delete hooks.exportedForTestingOnly.deprecationWarned[hookFnName]; + }); + + const makeHook = (ret) => { + return { + hook_name: hookName, + // Many tests will likely want to change this. Unfortunately, we can't use a convenience + // wrapper like `(...args) => hookFn(..args)` because the hooks look at Function.length and + // change behavior depending on the number of parameters. + hook_fn: (hn, ctx, cb) => cb(ret), + hook_fn_name: hookFnName, + part: {name: 'testPluginName'}, + }; + }; + + // Hook functions that should work for both synchronous and asynchronous hooks. + const supportedSyncHookFunctions = [ + { + name: 'return non-Promise value, with callback parameter', + fn: (hn, ctx, cb) => 'val', + want: 'val', + syncOk: true, + }, + { + name: 'return non-Promise value, without callback parameter', + fn: (hn, ctx) => 'val', + want: 'val', + syncOk: true, + }, + { + name: 'return undefined, without callback parameter', + fn: (hn, ctx) => {}, + want: undefined, + syncOk: true, + }, + { + name: 'pass non-Promise value to callback', + fn: (hn, ctx, cb) => { cb('val'); }, + want: 'val', + syncOk: true, + }, + { + name: 'pass undefined to callback', + fn: (hn, ctx, cb) => { cb(); }, + want: undefined, + syncOk: true, + }, + { + name: 'return the value returned from the callback', + fn: (hn, ctx, cb) => cb('val'), + want: 'val', + syncOk: true, + }, + { + name: 'throw', + fn: (hn, ctx, cb) => { throw new Error('test exception'); }, + wantErr: 'test exception', + syncOk: true, + }, + ]; + + describe('callHookFnSync', function() { + const callHookFnSync = hooks.exportedForTestingOnly.callHookFnSync; // Convenience shorthand. + + describe('basic behavior', function() { + it('passes hook name', async function() { + hook.hook_fn = (hn) => { assert.equal(hn, hookName); }; + callHookFnSync(hook); + }); + + it('passes context', async function() { + for (const val of ['value', null, undefined]) { + hook.hook_fn = (hn, ctx) => { assert.equal(ctx, val); }; + callHookFnSync(hook, val); + } + }); + + it('returns the value provided to the callback', async function() { + for (const val of ['value', null, undefined]) { + hook.hook_fn = (hn, ctx, cb) => { cb(ctx); }; + assert.equal(callHookFnSync(hook, val), val); + } + }); + + it('returns the value returned by the hook function', async function() { + for (const val of ['value', null, undefined]) { + // Must not have the cb parameter otherwise returning undefined will error. + hook.hook_fn = (hn, ctx) => ctx; + assert.equal(callHookFnSync(hook, val), val); + } + }); + + it('does not catch exceptions', async function() { + hook.hook_fn = () => { throw new Error('test exception'); }; + assert.throws(() => callHookFnSync(hook), {message: 'test exception'}); + }); + + it('callback returns undefined', async function() { + hook.hook_fn = (hn, ctx, cb) => { assert.equal(cb('foo'), undefined); }; + callHookFnSync(hook); + }); + + it('checks for deprecation', async function() { + sinon.stub(console, 'warn'); + hooks.deprecationNotices[hookName] = 'test deprecation'; + callHookFnSync(hook); + assert.equal(hooks.exportedForTestingOnly.deprecationWarned[hookFnName], true); + assert.equal(console.warn.callCount, 1); + assert.match(console.warn.getCall(0).args[0], /test deprecation/); + }); + }); + + describe('supported hook function styles', function() { + for (const tc of supportedSyncHookFunctions) { + it(tc.name, async function() { + sinon.stub(console, 'warn'); + sinon.stub(console, 'error'); + hook.hook_fn = tc.fn; + const call = () => callHookFnSync(hook); + if (tc.wantErr) { + assert.throws(call, {message: tc.wantErr}); + } else { + assert.equal(call(), tc.want); + } + assert.equal(console.warn.callCount, 0); + assert.equal(console.error.callCount, 0); + }); + } + }); + + describe('bad hook function behavior (other than double settle)', function() { + const promise1 = Promise.resolve('val1'); + const promise2 = Promise.resolve('val2'); + + const testCases = [ + { + name: 'never settles -> buggy hook detected', + // Note that returning undefined without calling the callback is permitted if the function + // has 2 or fewer parameters, so this test function must have 3 parameters. + fn: (hn, ctx, cb) => {}, + wantVal: undefined, + wantError: /UNSETTLED FUNCTION BUG/, + }, + { + name: 'returns a Promise -> buggy hook detected', + fn: () => promise1, + wantVal: promise1, + wantError: /PROHIBITED PROMISE BUG/, + }, + { + name: 'passes a Promise to cb -> buggy hook detected', + fn: (hn, ctx, cb) => cb(promise2), + wantVal: promise2, + wantError: /PROHIBITED PROMISE BUG/, + }, + ]; + + for (const tc of testCases) { + it(tc.name, async function() { + sinon.stub(console, 'error'); + hook.hook_fn = tc.fn; + assert.equal(callHookFnSync(hook), tc.wantVal); + assert.equal(console.error.callCount, tc.wantError ? 1 : 0); + if (tc.wantError) assert.match(console.error.getCall(0).args[0], tc.wantError); + }); + } + }); + + // Test various ways a hook might attempt to settle twice. (Examples: call the callback a second + // time, or call the callback and then return a value.) + describe('bad hook function behavior (double settle)', function() { + beforeEach(function() { + sinon.stub(console, 'error'); + }); + + // Each item in this array codifies a way to settle a synchronous hook function. Each of the + // test cases below combines two of these behaviors in a single hook function and confirms + // that callHookFnSync both (1) returns the result of the first settle attempt, and + // (2) detects the second settle attempt. + const behaviors = [ + { + name: 'throw', + fn: (cb, err, val) => { throw err; }, + rejects: true, + }, + { + name: 'return value', + fn: (cb, err, val) => val, + }, + { + name: 'immediately call cb(value)', + fn: (cb, err, val) => cb(val), + }, + { + name: 'defer call to cb(value)', + fn: (cb, err, val) => { process.nextTick(cb, val); }, + async: true, + }, + ]; + + for (const step1 of behaviors) { + // There can't be a second step if the first step is to return or throw. + if (step1.name.startsWith('return ') || step1.name === 'throw') continue; + for (const step2 of behaviors) { + // If step1 and step2 are both async then there would be three settle attempts (first an + // erroneous unsettled return, then async step 1, then async step 2). Handling triple + // settle would complicate the tests, and it is sufficient to test only double settles. + if (step1.async && step2.async) continue; + + it(`${step1.name} then ${step2.name} (diff. outcomes) -> log+throw`, async function() { + hook.hook_fn = (hn, ctx, cb) => { + step1.fn(cb, new Error(ctx.ret1), ctx.ret1); + return step2.fn(cb, new Error(ctx.ret2), ctx.ret2); + }; + + // Temporarily remove unhandled error listeners so that the errors we expect to see + // don't trigger a test failure (or terminate node). + const events = ['uncaughtException', 'unhandledRejection']; + const listenerBackups = {}; + for (const event of events) { + listenerBackups[event] = process.rawListeners(event); + process.removeAllListeners(event); + } + + // We should see an asynchronous error (either an unhandled Promise rejection or an + // uncaught exception) if and only if one of the two steps was asynchronous or there was + // a throw (in which case the double settle is deferred so that the caller sees the + // original error). + const wantAsyncErr = step1.async || step2.async || step2.rejects; + let tempListener; + let asyncErr; + try { + const seenErrPromise = new Promise((resolve) => { + tempListener = (err) => { + assert.equal(asyncErr, undefined); + asyncErr = err; + resolve(); + }; + if (!wantAsyncErr) resolve(); + }); + events.forEach((event) => process.on(event, tempListener)); + const call = () => callHookFnSync(hook, {ret1: 'val1', ret2: 'val2'}); + if (step2.rejects) { + assert.throws(call, {message: 'val2'}); + } else if (!step1.async && !step2.async) { + assert.throws(call, {message: /DOUBLE SETTLE BUG/}); + } else { + assert.equal(call(), step1.async ? 'val2' : 'val1'); + } + await seenErrPromise; + } finally { + // Restore the original listeners. + for (const event of events) { + process.off(event, tempListener); + for (const listener of listenerBackups[event]) { + process.on(event, listener); + } + } + } + assert.equal(console.error.callCount, 1); + assert.match(console.error.getCall(0).args[0], /DOUBLE SETTLE BUG/); + if (wantAsyncErr) { + assert(asyncErr instanceof Error); + assert.match(asyncErr.message, /DOUBLE SETTLE BUG/); + } + }); + + // This next test is the same as the above test, except the second settle attempt is for + // the same outcome. The two outcomes can't be the same if one step throws and the other + // doesn't, so skip those cases. + if (step1.rejects !== step2.rejects) continue; + + it(`${step1.name} then ${step2.name} (same outcome) -> only log`, async function() { + const err = new Error('val'); + hook.hook_fn = (hn, ctx, cb) => { + step1.fn(cb, err, 'val'); + return step2.fn(cb, err, 'val'); + }; + + const errorLogged = new Promise((resolve) => console.error.callsFake(resolve)); + const call = () => callHookFnSync(hook); + if (step2.rejects) { + assert.throws(call, {message: 'val'}); + } else { + assert.equal(call(), 'val'); + } + await errorLogged; + assert.equal(console.error.callCount, 1); + assert.match(console.error.getCall(0).args[0], /DOUBLE SETTLE BUG/); + }); + } + } + }); + }); + + describe('hooks.callAll', function() { + describe('basic behavior', function() { + it('calls all in order', async function() { + testHooks.length = 0; + testHooks.push(makeHook(1), makeHook(2), makeHook(3)); + assert.deepEqual(hooks.callAll(hookName), [1, 2, 3]); + }); + + it('passes hook name', async function() { + hook.hook_fn = (hn) => { assert.equal(hn, hookName); }; + hooks.callAll(hookName); + }); + + it('undefined context -> {}', async function() { + hook.hook_fn = (hn, ctx) => { assert.deepEqual(ctx, {}); }; + hooks.callAll(hookName); + }); + + it('null context -> {}', async function() { + hook.hook_fn = (hn, ctx) => { assert.deepEqual(ctx, {}); }; + hooks.callAll(hookName, null); + }); + + it('context unmodified', async function() { + const wantContext = {}; + hook.hook_fn = (hn, ctx) => { assert.equal(ctx, wantContext); }; + hooks.callAll(hookName, wantContext); + }); + }); + + describe('result processing', function() { + it('no registered hooks (undefined) -> []', async function() { + delete plugins.hooks.testHook; + assert.deepEqual(hooks.callAll(hookName), []); + }); + + it('no registered hooks (empty list) -> []', async function() { + testHooks.length = 0; + assert.deepEqual(hooks.callAll(hookName), []); + }); + + it('flattens one level', async function() { + testHooks.length = 0; + testHooks.push(makeHook(1), makeHook([2]), makeHook([[3]])); + assert.deepEqual(hooks.callAll(hookName), [1, 2, [3]]); + }); + + it('filters out undefined', async function() { + testHooks.length = 0; + testHooks.push(makeHook(), makeHook([2]), makeHook([[3]])); + assert.deepEqual(hooks.callAll(hookName), [2, [3]]); + }); + + it('preserves null', async function() { + testHooks.length = 0; + testHooks.push(makeHook(null), makeHook([2]), makeHook([[3]])); + assert.deepEqual(hooks.callAll(hookName), [null, 2, [3]]); + }); + + it('all undefined -> []', async function() { + testHooks.length = 0; + testHooks.push(makeHook(), makeHook()); + assert.deepEqual(hooks.callAll(hookName), []); + }); + }); + }); + + describe('callHookFnAsync', function() { + const callHookFnAsync = hooks.exportedForTestingOnly.callHookFnAsync; // Convenience shorthand. + + describe('basic behavior', function() { + it('passes hook name', async function() { + hook.hook_fn = (hn) => { assert.equal(hn, hookName); }; + await callHookFnAsync(hook); + }); + + it('passes context', async function() { + for (const val of ['value', null, undefined]) { + hook.hook_fn = (hn, ctx) => { assert.equal(ctx, val); }; + await callHookFnAsync(hook, val); + } + }); + + it('returns the value provided to the callback', async function() { + for (const val of ['value', null, undefined]) { + hook.hook_fn = (hn, ctx, cb) => { cb(ctx); }; + assert.equal(await callHookFnAsync(hook, val), val); + assert.equal(await callHookFnAsync(hook, Promise.resolve(val)), val); + } + }); + + it('returns the value returned by the hook function', async function() { + for (const val of ['value', null, undefined]) { + // Must not have the cb parameter otherwise returning undefined will never resolve. + hook.hook_fn = (hn, ctx) => ctx; + assert.equal(await callHookFnAsync(hook, val), val); + assert.equal(await callHookFnAsync(hook, Promise.resolve(val)), val); + } + }); + + it('rejects if it throws an exception', async function() { + hook.hook_fn = () => { throw new Error('test exception'); }; + await assert.rejects(callHookFnAsync(hook), {message: 'test exception'}); + }); + + it('rejects if rejected Promise passed to callback', async function() { + hook.hook_fn = (hn, ctx, cb) => cb(Promise.reject(new Error('test exception'))); + await assert.rejects(callHookFnAsync(hook), {message: 'test exception'}); + }); + + it('rejects if rejected Promise returned', async function() { + hook.hook_fn = (hn, ctx, cb) => Promise.reject(new Error('test exception')); + await assert.rejects(callHookFnAsync(hook), {message: 'test exception'}); + }); + + it('callback returns undefined', async function() { + hook.hook_fn = (hn, ctx, cb) => { assert.equal(cb('foo'), undefined); }; + await callHookFnAsync(hook); + }); + + it('checks for deprecation', async function() { + sinon.stub(console, 'warn'); + hooks.deprecationNotices[hookName] = 'test deprecation'; + await callHookFnAsync(hook); + assert.equal(hooks.exportedForTestingOnly.deprecationWarned[hookFnName], true); + assert.equal(console.warn.callCount, 1); + assert.match(console.warn.getCall(0).args[0], /test deprecation/); + }); + }); + + describe('supported hook function styles', function() { + const supportedHookFunctions = supportedSyncHookFunctions.concat([ + { + name: 'legacy async cb', + fn: (hn, ctx, cb) => { process.nextTick(cb, 'val'); }, + want: 'val', + }, + // Already resolved Promises: + { + name: 'return resolved Promise, with callback parameter', + fn: (hn, ctx, cb) => Promise.resolve('val'), + want: 'val', + }, + { + name: 'return resolved Promise, without callback parameter', + fn: (hn, ctx) => Promise.resolve('val'), + want: 'val', + }, + { + name: 'pass resolved Promise to callback', + fn: (hn, ctx, cb) => { cb(Promise.resolve('val')); }, + want: 'val', + }, + // Not yet resolved Promises: + { + name: 'return unresolved Promise, with callback parameter', + fn: (hn, ctx, cb) => new Promise((resolve) => process.nextTick(resolve, 'val')), + want: 'val', + }, + { + name: 'return unresolved Promise, without callback parameter', + fn: (hn, ctx) => new Promise((resolve) => process.nextTick(resolve, 'val')), + want: 'val', + }, + { + name: 'pass unresolved Promise to callback', + fn: (hn, ctx, cb) => { cb(new Promise((resolve) => process.nextTick(resolve, 'val'))); }, + want: 'val', + }, + // Already rejected Promises: + { + name: 'return rejected Promise, with callback parameter', + fn: (hn, ctx, cb) => Promise.reject(new Error('test rejection')), + wantErr: 'test rejection', + }, + { + name: 'return rejected Promise, without callback parameter', + fn: (hn, ctx) => Promise.reject(new Error('test rejection')), + wantErr: 'test rejection', + }, + { + name: 'pass rejected Promise to callback', + fn: (hn, ctx, cb) => { cb(Promise.reject(new Error('test rejection'))); }, + wantErr: 'test rejection', + }, + // Not yet rejected Promises: + { + name: 'return unrejected Promise, with callback parameter', + fn: (hn, ctx, cb) => new Promise((resolve, reject) => { + process.nextTick(reject, new Error('test rejection')); + }), + wantErr: 'test rejection', + }, + { + name: 'return unrejected Promise, without callback parameter', + fn: (hn, ctx) => new Promise((resolve, reject) => { + process.nextTick(reject, new Error('test rejection')); + }), + wantErr: 'test rejection', + }, + { + name: 'pass unrejected Promise to callback', + fn: (hn, ctx, cb) => { + cb(new Promise((resolve, reject) => { + process.nextTick(reject, new Error('test rejection')); + })); + }, + wantErr: 'test rejection', + }, + ]); + + for (const tc of supportedSyncHookFunctions.concat(supportedHookFunctions)) { + it(tc.name, async function() { + sinon.stub(console, 'warn'); + sinon.stub(console, 'error'); + hook.hook_fn = tc.fn; + const p = callHookFnAsync(hook); + if (tc.wantErr) { + await assert.rejects(p, {message: tc.wantErr}); + } else { + assert.equal(await p, tc.want); + } + assert.equal(console.warn.callCount, 0); + assert.equal(console.error.callCount, 0); + }); + } + }); + + // Test various ways a hook might attempt to settle twice. (Examples: call the callback a second + // time, or call the callback and then return a value.) + describe('bad hook function behavior (double settle)', function() { + beforeEach(function() { + sinon.stub(console, 'error'); + }); + + // Each item in this array codifies a way to settle an asynchronous hook function. Each of the + // test cases below combines two of these behaviors in a single hook function and confirms + // that callHookFnAsync both (1) resolves to the result of the first settle attempt, and (2) + // detects the second settle attempt. + // + // The 'when' property specifies the relative time that two behaviors will cause the hook + // function to settle: + // * If behavior1.when <= behavior2.when and behavior1 is called before behavior2 then + // behavior1 will settle the hook function before behavior2. + // * Otherwise, behavior2 will settle the hook function before behavior1. + const behaviors = [ + { + name: 'throw', + fn: (cb, err, val) => { throw err; }, + rejects: true, + when: 0, + }, + { + name: 'return value', + fn: (cb, err, val) => val, + // This behavior has a later relative settle time vs. the 'throw' behavior because 'throw' + // immediately settles the hook function, whereas the 'return value' case is settled by a + // .then() function attached to a Promise. EcmaScript guarantees that a .then() function + // attached to a Promise is enqueued on the event loop (not executed immediately) when the + // Promise settles. + when: 1, + }, + { + name: 'immediately call cb(value)', + fn: (cb, err, val) => cb(val), + // This behavior has the same relative time as the 'return value' case because it too is + // settled by a .then() function attached to a Promise. + when: 1, + }, + { + name: 'return resolvedPromise', + fn: (cb, err, val) => Promise.resolve(val), + // This behavior has the same relative time as the 'return value' case because the return + // value is wrapped in a Promise via Promise.resolve(). The EcmaScript standard guarantees + // that Promise.resolve(Promise.resolve(value)) is equivalent to Promise.resolve(value), + // so returning an already resolved Promise vs. returning a non-Promise value are + // equivalent. + when: 1, + }, + { + name: 'immediately call cb(resolvedPromise)', + fn: (cb, err, val) => cb(Promise.resolve(val)), + when: 1, + }, + { + name: 'return rejectedPromise', + fn: (cb, err, val) => Promise.reject(err), + rejects: true, + when: 1, + }, + { + name: 'immediately call cb(rejectedPromise)', + fn: (cb, err, val) => cb(Promise.reject(err)), + rejects: true, + when: 1, + }, + { + name: 'return unresolvedPromise', + fn: (cb, err, val) => new Promise((resolve) => process.nextTick(resolve, val)), + when: 2, + }, + { + name: 'immediately call cb(unresolvedPromise)', + fn: (cb, err, val) => cb(new Promise((resolve) => process.nextTick(resolve, val))), + when: 2, + }, + { + name: 'return unrejectedPromise', + fn: (cb, err, val) => new Promise((resolve, reject) => process.nextTick(reject, err)), + rejects: true, + when: 2, + }, + { + name: 'immediately call cb(unrejectedPromise)', + fn: (cb, err, val) => cb(new Promise((resolve, reject) => process.nextTick(reject, err))), + rejects: true, + when: 2, + }, + { + name: 'defer call to cb(value)', + fn: (cb, err, val) => { process.nextTick(cb, val); }, + when: 2, + }, + { + name: 'defer call to cb(resolvedPromise)', + fn: (cb, err, val) => { process.nextTick(cb, Promise.resolve(val)); }, + when: 2, + }, + { + name: 'defer call to cb(rejectedPromise)', + fn: (cb, err, val) => { process.nextTick(cb, Promise.reject(err)); }, + rejects: true, + when: 2, + }, + { + name: 'defer call to cb(unresolvedPromise)', + fn: (cb, err, val) => { + process.nextTick(() => { + cb(new Promise((resolve) => process.nextTick(resolve, val))); + }); + }, + when: 3, + }, + { + name: 'defer call cb(unrejectedPromise)', + fn: (cb, err, val) => { + process.nextTick(() => { + cb(new Promise((resolve, reject) => process.nextTick(reject, err))); + }); + }, + rejects: true, + when: 3, + }, + ]; + + for (const step1 of behaviors) { + // There can't be a second step if the first step is to return or throw. + if (step1.name.startsWith('return ') || step1.name === 'throw') continue; + for (const step2 of behaviors) { + it(`${step1.name} then ${step2.name} (diff. outcomes) -> log+throw`, async function() { + hook.hook_fn = (hn, ctx, cb) => { + step1.fn(cb, new Error(ctx.ret1), ctx.ret1); + return step2.fn(cb, new Error(ctx.ret2), ctx.ret2); + }; + + // Temporarily remove unhandled Promise rejection listeners so that the unhandled + // rejections we expect to see don't trigger a test failure (or terminate node). + const event = 'unhandledRejection'; + const listenersBackup = process.rawListeners(event); + process.removeAllListeners(event); + + let tempListener; + let asyncErr; + try { + const seenErrPromise = new Promise((resolve) => { + tempListener = (err) => { + assert.equal(asyncErr, undefined); + asyncErr = err; + resolve(); + }; + }); + process.on(event, tempListener); + const step1Wins = step1.when <= step2.when; + const winningStep = step1Wins ? step1 : step2; + const winningVal = step1Wins ? 'val1' : 'val2'; + const p = callHookFnAsync(hook, {ret1: 'val1', ret2: 'val2'}); + if (winningStep.rejects) { + await assert.rejects(p, {message: winningVal}); + } else { + assert.equal(await p, winningVal); + } + await seenErrPromise; + } finally { + // Restore the original listeners. + process.off(event, tempListener); + for (const listener of listenersBackup) { + process.on(event, listener); + } + } + assert.equal(console.error.callCount, 1, + 'Got errors:\n' + + console.error.getCalls().map((call) => call.args[0]).join('\n')); + assert.match(console.error.getCall(0).args[0], /DOUBLE SETTLE BUG/); + assert(asyncErr instanceof Error); + assert.match(asyncErr.message, /DOUBLE SETTLE BUG/); + }); + + // This next test is the same as the above test, except the second settle attempt is for + // the same outcome. The two outcomes can't be the same if one step rejects and the other + // doesn't, so skip those cases. + if (step1.rejects !== step2.rejects) continue; + + it(`${step1.name} then ${step2.name} (same outcome) -> only log`, async function() { + const err = new Error('val'); + hook.hook_fn = (hn, ctx, cb) => { + step1.fn(cb, err, 'val'); + return step2.fn(cb, err, 'val'); + }; + const winningStep = (step1.when <= step2.when) ? step1 : step2; + const errorLogged = new Promise((resolve) => console.error.callsFake(resolve)); + const p = callHookFnAsync(hook); + if (winningStep.rejects) { + await assert.rejects(p, {message: 'val'}); + } else { + assert.equal(await p, 'val'); + } + await errorLogged; + assert.equal(console.error.callCount, 1); + assert.match(console.error.getCall(0).args[0], /DOUBLE SETTLE BUG/); + }); + } + } + }); + }); + + describe('hooks.aCallAll', function() { + describe('basic behavior', function() { + it('calls all asynchronously, returns values in order', async function() { + testHooks.length = 0; // Delete the boilerplate hook -- this test doesn't use it. + let nextIndex = 0; + const hookPromises = []; + const hookStarted = []; + const hookFinished = []; + const makeHook = () => { + const i = nextIndex++; + const entry = {}; + hookStarted[i] = false; + hookFinished[i] = false; + hookPromises[i] = entry; + entry.promise = new Promise((resolve) => { + entry.resolve = () => { + hookFinished[i] = true; + resolve(i); + }; + }); + return {hook_fn: () => { + hookStarted[i] = true; + return entry.promise; + }}; + }; + testHooks.push(makeHook(), makeHook()); + const p = hooks.aCallAll(hookName); + assert.deepEqual(hookStarted, [true, true]); + assert.deepEqual(hookFinished, [false, false]); + hookPromises[1].resolve(); + await hookPromises[1].promise; + assert.deepEqual(hookFinished, [false, true]); + hookPromises[0].resolve(); + assert.deepEqual(await p, [0, 1]); + }); + + it('passes hook name', async function() { + hook.hook_fn = async (hn) => { assert.equal(hn, hookName); }; + await hooks.aCallAll(hookName); + }); + + it('undefined context -> {}', async function() { + hook.hook_fn = async (hn, ctx) => { assert.deepEqual(ctx, {}); }; + await hooks.aCallAll(hookName); + }); + + it('null context -> {}', async function() { + hook.hook_fn = async (hn, ctx) => { assert.deepEqual(ctx, {}); }; + await hooks.aCallAll(hookName, null); + }); + + it('context unmodified', async function() { + const wantContext = {}; + hook.hook_fn = async (hn, ctx) => { assert.equal(ctx, wantContext); }; + await hooks.aCallAll(hookName, wantContext); + }); + }); + + describe('aCallAll callback', function() { + it('exception in callback rejects', async function() { + const p = hooks.aCallAll(hookName, {}, () => { throw new Error('test exception'); }); + await assert.rejects(p, {message: 'test exception'}); + }); + + it('propagates error on exception', async function() { + hook.hook_fn = () => { throw new Error('test exception'); }; + await hooks.aCallAll(hookName, {}, (err) => { + assert(err instanceof Error); + assert.equal(err.message, 'test exception'); + }); + }); + + it('propagages null error on success', async function() { + await hooks.aCallAll(hookName, {}, (err) => { + assert(err == null, `got non-null error: ${err}`); + }); + }); + + it('propagages results on success', async function() { + hook.hook_fn = () => 'val'; + await hooks.aCallAll(hookName, {}, (err, results) => { + assert.deepEqual(results, ['val']); + }); + }); + + it('returns callback return value', async function() { + assert.equal(await hooks.aCallAll(hookName, {}, () => 'val'), 'val'); + }); + }); + + describe('result processing', function() { + it('no registered hooks (undefined) -> []', async function() { + delete plugins.hooks[hookName]; + assert.deepEqual(await hooks.aCallAll(hookName), []); + }); + + it('no registered hooks (empty list) -> []', async function() { + testHooks.length = 0; + assert.deepEqual(await hooks.aCallAll(hookName), []); + }); + + it('flattens one level', async function() { + testHooks.length = 0; + testHooks.push(makeHook(1), makeHook([2]), makeHook([[3]])); + assert.deepEqual(await hooks.aCallAll(hookName), [1, 2, [3]]); + }); + + it('filters out undefined', async function() { + testHooks.length = 0; + testHooks.push(makeHook(), makeHook([2]), makeHook([[3]]), makeHook(Promise.resolve())); + assert.deepEqual(await hooks.aCallAll(hookName), [2, [3]]); + }); + + it('preserves null', async function() { + testHooks.length = 0; + testHooks.push(makeHook(null), makeHook([2]), makeHook(Promise.resolve(null))); + assert.deepEqual(await hooks.aCallAll(hookName), [null, 2, null]); + }); + + it('all undefined -> []', async function() { + testHooks.length = 0; + testHooks.push(makeHook(), makeHook(Promise.resolve())); + assert.deepEqual(await hooks.aCallAll(hookName), []); + }); + }); + }); +});