Skip to content

Allow app to be runnable with cluster - #33

Merged
sam-github merged 1 commit into
masterfrom
feature/app-runnable-with-cluster
Jan 7, 2014
Merged

Allow app to be runnable with cluster#33
sam-github merged 1 commit into
masterfrom
feature/app-runnable-with-cluster

Conversation

@sam-github

Copy link
Copy Markdown
Contributor

app.js is designed to be the package.main, so app can be required. If
its run as the main module (node app.js), then it also starts up.
Those features are desired, but the conditionality of startup in app.js
means there is way to start the app when run under a supervisor such as
strongloop/strong-supervisor or unitech/pm2. To fix:

  • Create a single line server.js which unconditionally starts
  • Add a .bin property to package.json so the app can be globally
    installed.
  • Exports the startup code as a function(require('app.js').main when
    app is being used as a module (so behaviour of node server.js
    is identical to node app.js).
  • Remove the app.js calls to strong-agent and strong-cluster-control,
    they conflict with the supervisor's.

@sam-github

Copy link
Copy Markdown
Contributor Author

Since this removes the internal clustering and monitoring, it shouldn't be merged until strong-supervisor is released.

To reproduce the not-startable with cluster, try something like:

require('strong-agent').profile();
var cluster = require('cluster');
if(cluster.isMaster) {
cluster.fork();
} else {
require("./app.js');
}

Or use pm2 or strong-supervisor.

@slnode

slnode commented Jan 1, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-workspace/161/

@ritch

ritch commented Jan 1, 2014

Copy link
Copy Markdown
Member

Could we move app.main into the loopback core lib?

The more we move out of app.js the more we can fix by just patching. On the other hand, the more obscure and complex the app startup is.

@bajtos

bajtos commented Jan 2, 2014

Copy link
Copy Markdown
Member

Could we move app.main into the loopback core lib?

The more we move out of app.js the more we can fix by just patching. On the other hand, the more obscure and complex the app startup is.

👍

I would also consider renaming the function to app.start or app.run.

@sam-github

Copy link
Copy Markdown
Contributor Author

Sure, I think app.js should consist of no code, other than a call to boot(), so I'm happy to move it into loopback, but, I read the loopback lib, and didn't see any evidence of this kind of console.log printing there.

FWIW, I think app.js should look like:

require('loopback').main();

and that main() should be clearly documented as a sequence of steps (kindof like the comments inline in the template now):

app.main = function() {
   this.boot()
  this.start()
}

Most customization could be done in boot files, we could auto-upgrade, new features would be automatically inherited by lb apps as long as they call app.main(), and if they needed to write their own main() for their specific loopback app, they are familiar enough with it to manually track future changes.

boot() could do something like:

function () {
for file in all files in sorted() order in boot/*.js:
  require(file)(this).
}

Adding, removing, customizing middleware or other aspects of start sequence can then be done by adding/removing files. This is trivial to script. yo loopback:use --swagger could be implemented as a write of a file like:

module.exports = require('loopback-swagger');

Right now, loopback has a fast cycle of new features, which is great, but there is no upgrade path for app.js, it looks like new features need to be added to app.js manually on upgrade, based on creating a new lb app, and manually copying new features over.

@sam-github

Copy link
Copy Markdown
Contributor Author

I tried to move code to loopback/appliction.js, but don't see how, will need suggestions.

Note that in order to print the explorer location, it needs to know the baseeUrl, and it needs to know the explorer was actually configured. I don't see how to know this after-the-fact, but if you all know of some kind of "the middleware is running" API, perhaps its possible. I think some kind of generic, "annotated route" mechanism might be needed, so that its possible to programmatically walk the top-level routes, and report them to console if they are "annotated".

I'll change the name to start.

@slnode

slnode commented Jan 3, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-workspace/180/

@bajtos

bajtos commented Jan 3, 2014

Copy link
Copy Markdown
Member

Note that in order to print the explorer location, it needs to know the baseUrl, and it needs to know the explorer was actually configured. I don't see how to know this after-the-fact, but if you all know of some kind of "the middleware is running" API, perhaps its possible. I think some kind of generic, "annotated route" mechanism might be needed, so that its possible to programmatically walk the top-level routes, and report them to console if they are "annotated".

Perhaps we can use app.get() and app.set() as a short-term workaround?

// try to load loopback-explorer
if (explorerConfigured) app.set('looopback-explorer:path', explorerPath);

// inside app.run
var explorerPath = app.get('loopback-explorer:path');
if (explorerPath) // has explorer installed

I have encountered a similar issue in strongloop/loopback-example-access-control#5, where I would like to access apiPath.

@bajtos bajtos closed this Jan 3, 2014
@bajtos bajtos reopened this Jan 3, 2014
@slnode

slnode commented Jan 3, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-workspace/183/

@sam-github

Copy link
Copy Markdown
Contributor Author
// try to load loopback-explorer
if (explorerConfigured) app.set('looopback-explorer:path', explorerPath);

// inside app.run
var explorerPath = app.get('loopback-explorer:path');
if (explorerPath) // has explorer installed

The first chunk of code would be in app.js, the second in loopback core module. Having code in loopback core that depends on code in the loopback-workspace app template seems not so great, but if you are sure, I'll do.

This comment was lost in the close/reopen, I think:

miroslav: Another option is to move this functionality into the explorer module itself. app.start() can emit start
event and loopback-explorer

Some such loopback mechanism sounds great.

Integrating such a mechanism in loopback is outside of my scope of SLN-683, I'm just removing the no longer needed code from the various templates and samples we have, so that they can run using (any) external supervisor, and to remove coupling of them to agent/cluster control.

Maybe its in scope for example-access-control?

@bajtos

bajtos commented Jan 6, 2014

Copy link
Copy Markdown
Member

I deleted my comment because I realised it's not that easy.

There are two parts to this problem:

  1. When loopback-explorer is not loaded, we want to tell the user how to install it. Because the module was not loaded, it cannot print this info. Fortunately, the info can be printed by the code that was trying to load the module.
  2. When looopback-explorer is loaded, we want to report the explorer URL. The URL is based on the host/port where the app listen on. It's true that a start event emitted by the app is an elegant solution that allows us to decouple app.start() from plugin messages.

I am moving the discussion related to loopback-explorer to a new issue #34.

@bajtos

bajtos commented Jan 6, 2014

Copy link
Copy Markdown
Member

Let's move this pull request forward. IMO the changes look good and can be merged. Refactoring of app.start() to loopback core should be a new pull request.

@ritch any objections?

@bajtos

bajtos commented Jan 6, 2014

Copy link
Copy Markdown
Member

test please

@slnode

slnode commented Jan 6, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-workspace/186/

@ritch

ritch commented Jan 6, 2014

Copy link
Copy Markdown
Member

Sounds good.

We'll make a more structured change (might be loopback 1.5) to solidify app.js.

@sam-github

Copy link
Copy Markdown
Contributor Author

Thanks for the comments, I'll merge this afternoon.

@slnode

slnode commented Jan 6, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-workspace/197/

app.js is designed to be the package.main, so app can be required. If
its run as the main module (`node app.js`), then it also starts up.
Those features are desired, but the conditionality of startup in app.js
means it is difficult to start the app when run under a supervisor such
as strongloop/strong-supervisor or unitech/pm2. To fix:

- Create a single line server.js which unconditionally starts
- Add a .bin property to package.json so the app can be globally
  installed.
- Exports the startup code as a function(`require('app.js').main` when
  app is being used as a module (so behaviour of `node server.js` is
  identical to `node app.js`).
@slnode

slnode commented Jan 7, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-workspace/225/

@bajtos

bajtos commented Jan 7, 2014

Copy link
Copy Markdown
Member

LGTM 👍

sam-github added a commit that referenced this pull request Jan 7, 2014
@sam-github
sam-github merged commit e7214ee into master Jan 7, 2014
@sam-github
sam-github deleted the feature/app-runnable-with-cluster branch January 7, 2014 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants