Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin X missing dependency Y in server if manifest.plugins key order not carefully managed #1769

Closed
garthk opened this issue Jul 10, 2014 · 0 comments
Assignees
Labels
Milestone

Comments

@garthk
Copy link

@garthk garthk commented Jul 10, 2014

hapi -c crashes if plugin x registers with plugin.dependency('y') and manifest.json declares x before y:

{
  "plugins": {
    "x": {},
    "y": {}
  },
  ...
}

Consider Pack.register with two calls to console.error jammed in:

internals.Pack.prototype.register = function (plugins /*, [options], callback */) {
    console.error('===== Pack.register', plugins.plugin.register.attributes.name || plugins.plugin.register.attributes.pkg.name);
    var options = (typeof arguments[1] === 'object' ? arguments[1] : {});
    var callback = (typeof arguments[1] === 'object' ? arguments[2] : arguments[1]);

    var state = {
        dependencies: []
    };

    return this._register(plugins, options, state, function (err) {
        if (err) {
            return callback(err);
        }

        console.error('===== Pack.register callback dep checks');
        for (var i = 0, il = state.dependencies.length; i < il; ++i) {
            var dependency = state.dependencies[i];
            for (var s = 0, sl = dependency.servers.length; s < sl; ++s) {
                var server = dependency.servers[s];
                for (var d = 0, dl = dependency.deps.length; d < dl; ++d) {
                    var dep = dependency.deps[d];
                    Hoek.assert(server._registrations[dep], 'Plugin', dependency.plugin, 'missing dependency', dep, 'in server:', server.info.uri);
                }
            }
        }

        return callback();
    });
};

Output if I declare yar first in manifest.json:

===== Pack.register yar
===== Pack.register callback dep checks
===== Pack.register myAuthenticationPluginWhichNeedsYar
===== Pack.register callback dep checks

if I alpha-sort the keys to the manifest's plugins, Hapi registers myAuthenticationPluginWhichNeedsYar before yar and crashes with the 'missing dependency' assertion.

The documentation suggests circular requirements are OK if at most one plugin declares an after method:

after - an optional function called after all the specified dependencies have been registered and before the servers start. The function is only called if the pack servers are started. If a circular dependency is created, the call will assert (e.g. two plugins each has an after function to be called after the other).

I'm thus surprised Hapi checks the dependencies on every register, not just the last.

Reasons to argue this is not OK:

  • Not all JSON-handling tools are guaranteed to preserve order, as the standard does not require order preservation:

    An object is an unordered collection of zero or more name/value pairs, where a name is a string and a value is a string, number, boolean, null, object, or array.

  • This might make circular plugin dependencies impossible when using hapi -c for the "deploy with two files" (manifest and package) strategy I've heard @hueniverse rightly spruik more than once

  • It feels brittle

Reasons to argue this is OK:

  • V8 enumerates keys in insertion order as long as the keys don't parse as integers.
  • Who cares about hapi -c?
  • Who cares about my feels?
@hueniverse hueniverse added the bug label Jul 14, 2014
@hueniverse hueniverse added this to the 6.1.0 milestone Jul 14, 2014
@hueniverse hueniverse self-assigned this Jul 14, 2014
hueniverse pushed a commit that referenced this issue Jul 14, 2014
hueniverse pushed a commit that referenced this issue Jul 16, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants