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

feat: Add optional keepAlive option to Meter and Timer #64

Merged
merged 1 commit into from
May 24, 2019

Conversation

tuckbick
Copy link
Contributor

This PR unrefs timers by default, eliminating headaches for people unsure about why their unit tests keep hanging.

It also eliminates having to break a 1 line piece of code into 3:

// no unref-ing
collection.meter('responses').mark()

// with unref-ing
const responsesMeter = collection.meter('responses');
responsesMeter.unref();
responsesMeter.mark();

// with PR, keepAlive = false
collection.meter('responses').mark()

// with PR, keepAlive = true
collection.meter('responses', {keepAlive: true}).mark()

I've set the default to false since that was most helpful for my team. However, I could be convinced to have it default to true.

@@ -92,7 +92,8 @@ class Reporter {
* @type {Logger}
* @protected
*/
this._log = options.logger || consoleLogLevel({ name: 'Reporter', level: options.logLevel || 'info', prefix: prefix });
this._log =
options.logger || consoleLogLevel({ name: 'Reporter', level: options.logLevel || 'info', prefix: prefix });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier made this change.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 374

  • 6 of 6 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 89.042%

Totals Coverage Status
Change from base Build 372: 0.4%
Covered Lines: 759
Relevant Lines: 835

💛 - Coveralls

@csabapalfi csabapalfi merged commit 6283bf0 into yaorg:master May 24, 2019
@@ -22,6 +22,10 @@ class Meter {
constructor(properties) {
this._properties = properties || {};
this._initializeState();

if (!this._properties.keepAlive) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the consequences of making this the default behavior rather than inverting this logic?
Is this a breaking change for people?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean having the default be false.

Copy link
Contributor

@csabapalfi csabapalfi May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an application or process that has any kind of active handle (e.g. listening on a port, other timer etc) that'll keep the event loop alive anyway.

If measured-core is used in one-off jobs or similar then this behavior usually causes issues and keeps the process hanging.

I see it as highly unlikely to be breaking.

If you see this as a problem I can certainly unpublish 1.43.0 and release this as a major but to me this very much is a minor.

@tuckbick tuckbick deleted the unref branch May 24, 2019 20:38
This pull request was closed.
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