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.

Sign up for GitHub

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

Jump to bottom

lib: fix undefined timeout regression #3331

Closed
wants to merge 1 commit into from
Closed

lib: fix undefined timeout regression #3331

wants to merge 1 commit into from

Conversation

rmg
Copy link
Contributor

@rmg rmg commented Oct 12, 2015

63644dd introduced a regression caused by everyone's favourite
JavaScript feature: undefined < 0 === undefined >= 0.

Add a case to the existing tests to cover this scenario and then add
the check for undefined that makes the test pass.

@indutny @jasnell this should fix at least one of the v4.2.0 regressions.

evanlucas
evanlucas reviewed Oct 12, 2015
View reviewed changes
lib/timers.js
@@ -30,7 +30,7 @@ var lists = {};
// with them.
exports.active = function(item) {
const msecs = item._idleTimeout;
if (msecs < 0) return;
if (msecs < 0 || util.isUndefined(msecs)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of util.isUndefined, should be probably just be msecs === undefined

Copy link
Member

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing, incoming..

@indutny
Copy link
Member

indutny commented Oct 12, 2015

LGTM, cc @jasnell

@indutny indutny added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. land-on-v4.x labels Oct 12, 2015
@evanlucas
Copy link
Contributor

yea, LGTM other than that

@rmg
Copy link
Contributor Author

rmg commented Oct 12, 2015

Fixed the msecs === undefined nit and added a missing trailing ,. @indutny @evanlucas better?

@indutny
Copy link
Member

indutny commented Oct 12, 2015

LGTM

indutny pushed a commit that referenced this pull request Oct 12, 2015
@rmg @indutny
lib: fix undefined timeout regression
bde32f8 
63644dd introduced a regression caused by everyone's favourite
JavaScript feature: undefined < 0 === undefined >= 0.

Add a case to the existing tests to cover this scenario and then add
the check for undefined that makes the test pass.

PR-URL: #3331
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed By: Evan Lucas <evanlucas@me.com>
@evanlucas
Copy link
Contributor

are we doing trailing commas?

@indutny
Copy link
Member

indutny commented Oct 12, 2015

Landed in bde32f8. Forgot to run CI :(

Thank you!

@indutny
Copy link
Member

indutny commented Oct 12, 2015

Argh, I have overlooked it. No, we don't

@rmg
Copy link
Contributor Author

rmg commented Oct 12, 2015

sorry! should I bother fixing?

@rmg
lib: fix undefined timeout regression
1e14233 
63644dd introduced a regression caused by everyone's favourite
JavaScript feature: undefined < 0 === undefined >= 0.

Add a case to the existing tests to cover this scenario and then add
the check for undefined that makes the test pass.
@rmg
Copy link
Contributor Author

rmg commented Oct 12, 2015

guess I probably shouldn't have force pushed the fix for the trailing comma.. oh well.

@Trott
Copy link
Member

Trott commented Oct 13, 2015

I'm impressed/terrified that this edge case surfaced so quickly. Thanks for finding/fixing.

@dougwilson dougwilson mentioned this pull request Oct 13, 2015
Closed
jasnell pushed a commit that referenced this pull request Oct 13, 2015
@rmg @jasnell
lib: fix undefined timeout regression
c245a19 
63644dd introduced a regression caused by everyone's favourite
JavaScript feature: undefined < 0 === undefined >= 0.

Add a case to the existing tests to cover this scenario and then add
the check for undefined that makes the test pass.

PR-URL: #3331
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed By: Evan Lucas <evanlucas@me.com>
@jasnell
Copy link
Member

jasnell commented Oct 13, 2015

Landed in v4.x in c245a19

@jasnell jasnell closed this Oct 13, 2015
jasnell added a commit that referenced this pull request Oct 13, 2015
@jasnell
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
2f39185 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)
@jasnell jasnell mentioned this pull request Oct 13, 2015
Closed
jasnell added a commit that referenced this pull request Oct 13, 2015
@jasnell
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
9ddb251 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)
@Fishrock123
Copy link
Contributor

Oops.

@chrisabrams
Copy link

Thanks for this - really appreciate the quick turn around for those of us relying on this fix and using LTS!

@rmg
Copy link
Contributor Author

rmg commented Oct 13, 2015

@Trott I'm really aggressive with rolling new released out to StrongLoop's CI, and we have over 100 modules running through it.. didn't take long before we started getting some really mysterious build failures.

@rmg rmg deleted the fix-undefined-timeouts branch October 13, 2015 16:03
jasnell added a commit that referenced this pull request Oct 13, 2015
@jasnell
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
659291c 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)

* Document an additional known issue with pipelined requests
  - See: #3332 and #3342
jasnell added a commit that referenced this pull request Oct 13, 2015
@jasnell
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
0a7076b 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)

* Document an additional known issue with pipelined requests
  - See: #3332 and #3342
@Starefossen Starefossen mentioned this pull request Oct 13, 2015
Merged
@rmg rmg mentioned this pull request Oct 15, 2015
Closed
This was referenced Feb 20, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Sep 25, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 26, 2022
Open
@137717unity 137717unity mentioned this pull request Sep 26, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Sep 27, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 29, 2022
Open
This was referenced Sep 29, 2022
Open
Open
This was referenced Oct 5, 2022
Open
Open
@snyk-bot snyk-bot mentioned this pull request Oct 6, 2022
Open
@137717unity 137717unity mentioned this pull request Oct 7, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 7, 2022
Open
This was referenced Oct 8, 2022
Open
Open
@137717unity 137717unity mentioned this pull request Oct 9, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Oct 9, 2022
Open
@kissofire kissofire mentioned this pull request Jun 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rmg @indutny @evanlucas @Trott @jasnell @Fishrock123 @chrisabrams @MylesBorins