Hapi: Assert when server.start() is called before server.register() is done

[hulbert]

As far as I can tell, server.register doesn't wait for the previous plugin to call the next callback. This is pretty confusing since next() seems to imply it's safe to run async code in a plugin that a later plugin would be dependent on.

[garthk]

Figured out in IRC @hulbert had not waited for server.register to call back before calling server.start. Could we have start throw an exception if someone tries that?

[hulbert]

@garthk talked me through some of this on IRC; as a "new user" I'm hoping to just offer some general feedback about what's confusing:

1. AFAICT doing something like server.register({ plugin: require('plugin1') }, { plugin: require('plugin2') }, callback) means that plugin2 will wait on plugin1's next() call.
2. Doing the following means plugin2 will _not_ wait on plugin1's callback

server.register({ plugin: require('plugin1') });
server.register({ plugin: require('plugin2') });

1. To register a plugin with a prefix, you can't pass an array to server.register because the prefix applies to all the plugins you're registering.
2. Because of this, to do sequential registration of plugins that require different prefixes you essentially need to enter callback hell (or promisify server.register which is what I've done).

Now, some of this may not matter since route registration isn't really async as far as I can tell. However, hopefully you can see the confusion with how plugin registration currently works. @garthk mentioned using server.dependency but this isn't mentioned in the tutorials so I didn't notice its existence. It also is unclear if it would be recommended to declare dependencies in routing-related plugins which could depend on almost all plugins (or any plugins that behave like middleware, like auth, response manipulation, etc)

[Marsup]

I believe most people will use glue if they're uncomfortable with what you're calling callback hell.

[garthk]

The examples implicitly guide new users to server = new Server. If we want them to use glue, we should tell them that.

[hueniverse]

Is this a matter of the register() callback being optional?

[garthk]

Not quite. The callback isn't optional, at least in 8.4 and 8.5: we assert typeof callback === 'function' in Plugin.register.

The problem is:

• We assume the user will wait for server.register to call back before attempting server.start
• The documentation and tutorials do not encourage them to do so
• If they don't, it can bite them

Consider this example code from hapijs.com/tutorials/plugins:

// load one plugin
server.register({register: require('myplugin')}, function (err) {
    if (err) {
        console.error('Failed to load plugin:', err);
    }
});

Or, the 8.5.2 API Reference for server.register:

server.register({
    register: require('plugin_name'),
    options: {
        message: 'hello'
    }
 }, function (err) {

     if (err) {
         console.log('Failed loading plugin');
     }
 });

Neither mention server.start, leaving users free to conclude they can call server.start right after server.register — just like @hulbert did. It'll _seem_ to work fine, until you try to use anything the plugin set up, e.g. by trying to call a server method at startup to warm the cache.

A rude but simple fix would be to have server.start crash or call back with an error if any plugin registration function hasn't called next yet. That'd be consistent with the way we Hoek.assert many expectations at API boundaries.

A more polite but complicated fix would be to have server.start wait for the plugin's registration functions to call back, which I think we already do for the after arguments to server.dependency. Here's a failing test for that approach:

lab.test('server.start waits for plugin registration functions to call back', function (done) {
    function plugin(_server, options, next) {
        setImmediate(function() {
            _server.expose('key', 'value');
            next();
        });
    }

    plugin.attributes = { name: 'plugin' };

    var server = new Hapi.Server();
    server.connection({ port: 0 });
    server.register(plugin, function (err) {
        expect(!err).to.be.true();
    });

    server.start(function started() {
        expect(server.plugins).to.deep.include({
            plugin: {
                key: 'value'
            }
        });
        done();
    });
});

[hueniverse]

Making start() wait for register() is going to be a debugging nightmare when things don't work and you can't figure out what it is stuck on.

I like the idea of throwing when start() is called when register() hasn't finished with an option to ignore that behavior.

[hueniverse]

Decide not to implement an override for now.

[boneskull]

FWIW, from above, this bit / is biting me:

server.register({ plugin: require('plugin1') }, { plugin: require('plugin2') }, callback) means that plugin2 will wait on plugin1's next() call.

Doing the following means plugin2 will not wait on plugin1's callback

server.register({ plugin: require('plugin1') });
server.register({ plugin: require('plugin2') });

My problem isn't waiting for start(), it's that if plugin2 depends on plugin1, things go awry.

What would be a good solution for this? I would expect singular and multiple calls to server.register() to work the same.

[devinivy]

You can indicate that plugin2 depends on plugin1 using server.dependency: http://hapijs.com/api#serverdependencydependencies-after.

[boneskull]

Yes, but that will fail unless I do this:

server.register(pluginA, function() {
  server.register(pluginB, function() {
    // etc
  });
});

which I suppose is OK if I'm using the async package. I haven't tried glue

[hueniverse]

This is why we have glue and why I have been supportive of others to go and create new ways of building a server and loading plugins. The core API gives you the functionality you need, but beyond that, you need to build your own utilities to make life easier.

[boneskull]

OK, thanks

[rainabba]

I'm here for similar reasons though I found my way through loading plugins and calling start. Where I'm having trouble is with lab. My server takes a few seconds to fire up thanks to the sync loading of certain plugins which depend on each other. My lab tests inject() too quickly and I can't figure out how to make the test wait at all (even with lab.before() and setTimeout() ) and it seems like there should be a good way to setup my tests to wait until server.start() has been called at the least.

Any suggestions? Custom event added to my exported server object was one idea i had. A callback into that require is another, but I don't see how to take advantage of either for actually starting the testing given the way that the test export is setup.

[devinivy]

@rainabba feel free to open an issue over in https://github.com/hapijs/discuss about this. To get you started, though, check out the usage section of the lab docs here, which provides an example how to wait for an asynchronous operation in lab.before(cb)

[hulbert]

@rainabba Here's an example of what we do since I had this issue with async server starting before testing. This could be adapted to callbacks too. The promisification is compliments of Bluebird.

// will return a promise; this is in server.js
function initServer() {
    let server = new Hapi.Server(serverOptions)
    let serverRegister = Promise.promisify(server.register, server)
    let serverStart = Promise.promisify(server.start, server)

    return serverRegister([
        { register: SomeMiddleware }
    ])
    .then(function() {
        return serverStart()
    })
}

// now can `node server.js` or import this file
if (!module.parent) {
    initServer()
}
module.exports = initServer

Then, in a lab test you can (simplified example):

var initServer = require('./server.js')

lab.before(function(done) {
    initServer().asCallback(done)
})

[rainabba]

@hulbert Just wanted to say thanks for the example. I think I have to restructure my app to properly expose the appropriate promise, but I see how it works and think it's exactly what I needed.

[PetrSnobelt]

@hulbert Thank you for your example, but how you access server.inject when server is not returned by initServer

[rainabba]

@PetrSnobelt Here is the approach I came to thanks to the suggestion from @hulbert . I think it will address your need.

Your node app:

function initServer() {

    return new Promise ( function( resolveInitServer, rejectInitServer ) {

        var server = new Hapi.Server( options ),
            Plugins = [];

        server.register(Plugins, function(err) {
            if (err) {
                throw err;
            }

            server.start(function(err) {

                if (err) {
                    throw err;
                }

                resolveInitServer(server);
            });

        }

    }
}

if ( !module.parent ) {
    initServer().then( function(res) {
        console.warn( 'initServer promise resolved.');
    });
}

module.exports = initServer;

Your test.js:

var Code = require('code'),
    Lab = require('lab'),
    lab = exports.lab = Lab.script(),
    describe = lab.describe,
    it = lab.it,
    before = lab.before,
    after = lab.after,
    expect = Code.expect,
    server = null;

lab.experiment("Test Username Existence", function() {

    lab.before(function(done) {
        initServer().then(function(res) {
            server = res;
            done();
        });
    });

    lab.test("Test Description", function(done) {
        server.inject({
            method: someVerb,
            url: someUrl,
            payload: {
                "foo": "bar"
            }
        }, function(response) {
            var result = response.result;
            Code.expect(response.statusCode).to.equal(200);
            Code.expect(response.result).to.be.an.object();
            Code.expect(response.result.action == "subscribe").to.be.true();
            Code.expect(response.result.email).to.be.a.string();
            Code.expect(response.result.email == unitTestEmail).to.be.true();
            done();
        });
    });

}

[PetrSnobelt]

Interesting, it looks like you don't define server property/object in your test.
I ended with returning server from last .then in initServer promise for now

[rainabba]

@PetrSnobelt Oops. I tried to dumb down my code and now see why I did something in my code that wasn't making sense when I setup those examples. See my updated version. In the app, server is passed to resolveInitServer(). In the test code, that's caught inside lab.before with the handler for initServer().then() and from there assigned to a global that was initialized with null so it's available to the tests that follow.

[hulbert]

@PetrSnobelt yes we're returning the server last in the promise chain from the exported function (basically an extra then from my example above)

[jmm]

The approach to tracking the plugin registration status for throwing on server.start() has a flaw. The status can be clobbered by later calls to server.register() when earlier plugins haven't finished. See https://github.com/hapijs/hapi/pull/3333.

Even so, the relationship between server.register() and server.start() is prone to race conditions. If you also have some other async prerequisite to calling server.start() you can get different results on different runs (sometimes the error on server.start(), sometimes not) unless you manage it yourself somehow, which is kind of a pain. I think it'd be nice to have something like this:

// Promise that settles based on all server.register() calls that have already
// happened (maybe make additional calls to server.register() throw after this):
server.ready()
.then(function () {
  return server.start();
})

[mindvox]

I think the methods suggested should be included in the documentation.

[fabiomotta0311]

'use strict'

const Hapi = require('hapi')
const server = new Hapi.Server()

server.connection({ port: 8080, host: '0.0.0.0'})

server.register({
register: require('hapi-router'),
options: {
routes: 'resources/**/routes.js'
}
}).then(function() {
return serverStart()
})

function serverStart() {
server.start(err => {
if (err) throw err
console.log('Servidor iniciando em %s', server.info.uri)
})
}

[lock[bot]]

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.