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

[RFR][MINOR] Use Env Variable to configure Max Listeners at runtime #939

Conversation

@slnode
Copy link

slnode commented May 20, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@chrisamoore
Copy link
Author

👍 Bumpy !

@bajtos
Copy link
Member

bajtos commented May 23, 2016

@slnode ok to test

@bajtos
Copy link
Member

bajtos commented May 23, 2016

@chrisamoore thank you for the pull request. I am not comfortable reading ENV variables so deep inside loopback code. A much cleaner approach is to expose API allowing LoopBack apps to set the MaxListener limit, and then optionally set this limit from ENV in loopback-boot.

Depending on your use cases, I can imagine different options for such API:

  1. Simplest to implement: a model setting. Down side: users have to change this value for each model, and cannot change it for models they don't control (e.g. loopback-workspace models).

Implementation:

// in lib/model-builder.js
events.setMaxListeners(ModelClass.settings.maxEventListeners || 32);

Usage:

// common/models/my-model.json
{
  "name": "MyModel",
  "base": "PersistedModel",
  "maxEventListeners": 64,
  "properties": {
      // ...
  }
}
  1. A setting on modelBuilder instance, exposed via loopback app.registry object.

Implementation:

// in juggler's lib/model-builder.js
events.setMaxListeners(this.maxModelEventListeners || 32);

// in loopback-boot
if (process.env.LB_MAX_LISTENERS) {
  app.registry.modelBuilder.maxModelEventListeners = +process.env.LB_MAX_LISTENERS;
}

@chrisamoore @raymondfeng thoughts?

@bajtos bajtos self-assigned this May 23, 2016
@chrisamoore
Copy link
Author

The issue with the static nature of the model config is that it is always at that limit, where as with the ENV flag I can toggle it very simply for tests (which is bombing, and on an older project it kept bombing out due to the monolithic nature of the application) For my particular use case this is only for the nature of the tests we are writing which are exhaustive in nature. cc @bajtos

@BerkeleyTrue
Copy link
Contributor

I'd also like to see this in model-config instead of as a env variable.

Maybe a happy medium would be model-config with a env variable global override?

@chrisamoore
Copy link
Author

@bajtos @BerkeleyTrue Shall I amend the PR to reflect the comment of @BerkeleyTrue above ?

@chrisamoore
Copy link
Author

Also @bajtos the build looks a little funky in it's failures

@bajtos
Copy link
Member

bajtos commented May 24, 2016

Let me summarize my understanding.

In @chrisamoore's use case, one wants to set a single flag in a single place to change the maxListeners value in all models, in a way that makes it easy to change this value from the tests.

@BerkeleyTrue would like to change this value on per-model basis.

Maybe a happy medium would be model-config with a env variable global override?

Providing a setting at the model level with an option to override the value globally is fine with me. However, I am still opposed to control this directly via ENV variables here in juggler. As I wrote in #939 (comment), the global setting should be kept in ModelBuilder.

BTW I created a new issue to keep track of the requirement to configure modelBuilder settings via JSON files, see strongloop/loopback#2370.


For this pull request, I am proposing the following:

  1. Implement my suggestion described in [RFR][MINOR] Use Env Variable to configure Max Listeners at runtime #939 (comment) with one small change: store the flag in modelBuilder.options.maxModelEventListeners instead of modelBuilder.maxModelEventListeners to prevent possible clash between setting names and instance methods in the future.

This will allow @chrisamoore to handle MAX_LISTENERS ENV variable in his code, e.g. as follows:

// server/server.js
var app = loopback();
if (process.env.MAX_LISTENERS) {
  app.registry.modelBuilder.settings.maxModelEventListeners = +process.env.MAX_LISTENERS;
}
boot(app, __dirname);

Once strongloop/loopback#2370 is landed, one can configure max listeners e.g. this way:

// server/config.js
{
  "restApiRoot": "/api",
  // ...
  "modelBuilder": { 
    "maxModelEventListeners": "${MAX_LISTENERS}"
  }
}

or alternatively, assuming tests are run with NODE_ENV=test:

// server/config.test.js
{
  "modelBuilder": {
    "maxModelEventListeners": 256
  }
}

(Note that I am assuming the tests are loading the app by importing whole server/server.js.)

  1. To support @BerkeleyTrue's use case, modelBuilder can take into account model-level settings too. I am fine to keep that out of scope of this pull request though.

@chrisamoore
Copy link
Author

@bajtos would this need to include a PR to loopback as well to affect the server.js as well ?

@bajtos
Copy link
Member

bajtos commented May 25, 2016

would this need to include a PR to loopback as well to affect the server.js as well ?

I don't think so. For short-term, users of this new feature should edit their server/server.js manually in their projects. For long-term, I'd like to implement strongloop/loopback#2370.

Both ways, there is no need to change loopback now, this pull request can be landed on its own.

@bajtos
Copy link
Member

bajtos commented Jun 13, 2016

@chrisamoore ping, what's the status of this patch? Are you still keen to get it finished?

@bajtos
Copy link
Member

bajtos commented Oct 6, 2016

I am closing this pull request as abandoned. If there is anybody willing to take over this work, then feel free to send a new pull request. See #939 (comment) for guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants