This repository was archived by the owner on Feb 4, 2022. It is now read-only.
Artificial _called property set on Timeout instance... #248
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eout.isRunning() work properly. Resolves mongodb-js/mongodb-core#247.
…st@2.65.0 in deps. Improvement for mongodb-js/mongodb-core#247
ilguzin
changed the title
|
@daprahamian please take a look at this |
daprahamian
approved these changes
Dec 1, 2017
lib/topologies/shared.js
Outdated
| @@ -217,7 +217,8 @@ function Timeout(fn, time) { | |||
|
|
|||
| this.start = function () { | |||
| if (!this.isRunning()) { | |||
| timer = setTimeout(fn, time); | |||
| // The artificial _called is set here for compatibility with node.js 0.10.x/0.12.x versions | |||
| timer = setTimeout(function() { fn(); if (timer !== false && timer._called === undefined) { timer._called = true; } }, time); | |||
There was a problem hiding this comment.
Can you please incorporate the following changes:
- replace
timer !== falsewith justtimer - Break this all out into multiple lines.
Ideally, it would be something like this?
timer = setTimeout(function() {
fn();
if (timer && timer._called === undefined) {
timer._called = true;
}
}, time);
daprahamian
suggested changes
Dec 1, 2017
daprahamian
approved these changes
Dec 1, 2017
|
Any chance to see it in next release this month? |
|
@ilguzin there will be a 2.x release next week |
mbroadst
approved these changes
Dec 1, 2017
|
@mbroadst, cool! Thanks :-) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
... to have Timeout.isRunning() work properly for legacy node.js versions. Resolves mongodb-js/mongodb-core#247.
Fixing Timeout.isRunning was probably the best way to solve the issue and fit into the initial idea behind Timeout class.
Also returned back 0.10 and 0.12 targets for Travis. For that reason the 'request' lib has been fixed to @2.65.0 version in dev deps and added tests for the memory leak issue.
It does not seem to be reasonable to have these legacy targets to be turned off simply because of Hawk semver. That move has eventually played a dramatic role in hidding of the Timeout._called absence for v0.12 which was causing memory leak.