lib: fix undefined timeout regression by rmg · Pull Request #3331 · nodejs/node · GitHub
Skip to content

lib: fix undefined timeout regression#3331

Closed
rmg wants to merge 1 commit into
nodejs:masterfrom
rmg:fix-undefined-timeouts
Closed

lib: fix undefined timeout regression#3331
rmg wants to merge 1 commit into
nodejs:masterfrom
rmg:fix-undefined-timeouts

Conversation

@rmg

@rmg rmg commented Oct 12, 2015

Copy link
Copy Markdown
Contributor

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
Comment thread lib/timers.js Outdated

@evanlucas evanlucas Oct 12, 2015

Copy link
Copy Markdown
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

@indutny indutny Oct 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree.

@rmg rmg Oct 12, 2015

Copy link
Copy Markdown
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

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member

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

evanlucas commented Oct 12, 2015

Copy link
Copy Markdown
Contributor

yea, LGTM other than that

@rmg rmg force-pushed the fix-undefined-timeouts branch from d66d408 to e2c2b44 Compare October 12, 2015 23:18
@rmg

rmg commented Oct 12, 2015

Copy link
Copy Markdown
Contributor Author

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

@indutny

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member

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

evanlucas commented Oct 12, 2015

Copy link
Copy Markdown
Contributor

are we doing trailing commas?

@indutny

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member

Landed in bde32f8. Forgot to run CI :(

Thank you!

@indutny

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member

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

@rmg

rmg commented Oct 12, 2015

Copy link
Copy Markdown
Contributor Author

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 rmg force-pushed the fix-undefined-timeouts branch from e2c2b44 to 1e14233 Compare October 12, 2015 23:29
@rmg

rmg commented Oct 12, 2015

Copy link
Copy Markdown
Contributor Author

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

@Trott

Trott commented Oct 13, 2015

Copy link
Copy Markdown
Member

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

jasnell commented Oct 13, 2015

Copy link
Copy Markdown
Member

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

Fishrock123 commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

Oops.

@chrisabrams

chrisabrams commented Oct 13, 2015

Copy link
Copy Markdown

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

@rmg

rmg commented Oct 13, 2015

Copy link
Copy Markdown
Contributor Author

@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