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

timers: fix priority queue removeAt fn #23870

Closed
wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Oct 25, 2018

🤷‍♂️ I'm an idiot... and we probably should get a release out (11.0.1)... @nodejs/tsc

Fixes: #23860

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@apapirovski
Copy link
Member Author

apapirovski commented Oct 25, 2018

@targos
Copy link
Member

targos commented Oct 25, 2018

I suppose you want to fast-track?

I can promote the release today or tomorrow, if someone else prepares it.

@apapirovski apapirovski added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 25, 2018
@apapirovski
Copy link
Member Author

Please 👍 this if you approve fast tracking.

@apapirovski apapirovski added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 25, 2018
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM and +1 to fast track. Can you add a test with timers as well?

@Trott
Copy link
Member

Trott commented Oct 25, 2018

Commit pushed since last CI. CI again: https://ci.nodejs.org/job/node-test-pull-request/18147/

@apapirovski
Copy link
Member Author

@Trott technically I updated the CI after the commit :)

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

A fast-track is good. There are a few more commits landed in master that should be in an 11.0.1 release. Let's make sure we pull in the other semver-patch commits.

@Trott
Copy link
Member

Trott commented Oct 25, 2018

+1 to fast-track

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 25, 2018
PR-URL: nodejs#23870
Fixes: nodejs#23860
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 25, 2018

Landed in 958d5b7.

The request from @mcollina for an additional test can be added as a separate PR if highly desirable. I assumed it was an optional nit based on their approval. Hope that was OK.

@Trott Trott closed this Oct 25, 2018
@mcollina
Copy link
Member

mcollina commented Oct 25, 2018 via email

targos pushed a commit that referenced this pull request Oct 26, 2018
PR-URL: #23870
Fixes: #23860
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 27, 2018
targos added a commit that referenced this pull request Oct 28, 2018
Notable changes:

* deps
  * Updated ICU to 63.1. #23715
* repl
  * Top-level for-await-of is now supported in the REPL.
    #23841
* timers
  * Fixed an issue that could cause timers to enter an infinite loop.
    #23870

PR-URL: #23922
targos pushed a commit that referenced this pull request Nov 1, 2018
PR-URL: #23870
Fixes: #23860
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Nov 1, 2018
Notable changes:

* deps
  * Updated ICU to 63.1. #23715
* repl
  * Top-level for-await-of is now supported in the REPL.
    #23841
* timers
  * Fixed an issue that could cause timers to enter an infinite loop.
    #23870

PR-URL: #23922
targos added a commit that referenced this pull request Nov 2, 2018
Notable changes:

* deps
  * Updated ICU to 63.1. #23715
* repl
  * Top-level for-await-of is now supported in the REPL.
    #23841
* timers
  * Fixed an issue that could cause timers to enter an infinite loop.
    #23870

PR-URL: #23922
@podarok
Copy link

podarok commented Nov 15, 2018

Thank you for the fix.
I spent 3 days to blame node-red hanging at 100%CPU

@MylesBorins
Copy link
Contributor

I'm assuming this fix is based on top of a Semver-Major change and thus shouldn't land within LTS. Can you please change the labels to dont-land if that is an accurate assessment

@adrienbaron
Copy link

Took me a whole week of my API randomly crashing to figure out this was the issue! Was on Node 11.0.0. Thank you so much for the fix! Went back to LTS though ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. notable-change PRs with changes that should be highlighted in changelogs. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clearTimeout blocks the process (100% CPU usage)