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

Fix double initialisation when two appmetrics instances are started (singleton bug) #508

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

sjanuary
Copy link
Contributor

An issue was discovered with the implementation of the appmetrics global singleton. Essentially a lot of initialization code was being run twice including attaching the probes and the socket.io probe doesn't have a global check for whether it is already attached. This PR only runs the initialization code once.

See RuntimeTools/appmetrics-dash#124

@sjanuary sjanuary changed the title Fix singleton bug Fix double initialisation when two appmetrics instances are started (singleton bug) Nov 28, 2017
@codecov-io
Copy link

Codecov Report

Merging #508 into master will decrease coverage by 0.03%.
The diff coverage is 73.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage    60.8%   60.77%   -0.04%     
==========================================
  Files          49       49              
  Lines        3215     3215              
==========================================
- Hits         1955     1954       -1     
- Misses       1260     1261       +1
Impacted Files Coverage Δ
index.js 75% <73.71%> (ø) ⬆️
headless_zip.js 73.33% <0%> (-2.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b964189...eca0cfd. Read the comment docs.

@a-roberts
Copy link

Confirmed this fixes a problem seen here, thanks @sjanuary for the quick fix!

@gibfahn
Copy link
Contributor

gibfahn commented Nov 28, 2017

The ignore whitespace diff is much cleaner btw (for reviewers): https://github.com/RuntimeTools/appmetrics/pull/508/files?w=1

You could do an early return;, but I think the if/else is clearer.

Copy link
Member

@hhellyer hhellyer left a comment

Choose a reason for hiding this comment

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

Looks good and makes sense, we don't want to do any initialisation if appmetrics has already been required.
Node is single threaded so there's no synchronisation issues to worry about.

@hhellyer hhellyer merged commit 7eec260 into RuntimeTools:master Nov 30, 2017
@sjanuary sjanuary added this to the 4.0.0 milestone Jan 24, 2018
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.

5 participants