Skip to content

timers: fix priority queue removeAt fn #23870

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
Closed
wants to merge 1 commit into from
Closed

timers: fix priority queue removeAt fn #23870

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Contributor

@apapirovski apapirovski commented Oct 25, 2018 •
edited
Loading

🤷‍♂️ 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

@nodejs-github-bot
Copy link
Collaborator

@apapirovski build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1364/pipeline

@apapirovski
Copy link
Contributor Author

apapirovski commented Oct 25, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/18141/

@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
Contributor 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
mcollina
mcollina approved these changes Oct 25, 2018
View reviewed changes
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
Contributor Author

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

jasnell
jasnell approved these changes Oct 25, 2018
View reviewed changes
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
@apapirovski @Trott
timers: fix priority queue removeAt fn
958d5b7 
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
@apapirovski @targos
timers: fix priority queue removeAt fn
8f07bfb 
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]>
@richardlau richardlau mentioned this pull request Oct 27, 2018
Closed
@targos targos mentioned this pull request Oct 27, 2018
Merged
@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 27, 2018
@richardlau richardlau mentioned this pull request Oct 28, 2018
Closed
targos added a commit that referenced this pull request Oct 28, 2018
@targos
2018-10-30, Version 11.1.0 (Current)
3dfd630 
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
@xPaw xPaw mentioned this pull request Oct 29, 2018
Closed
targos pushed a commit that referenced this pull request Nov 1, 2018
@apapirovski @targos
timers: fix priority queue removeAt fn
167e99b 
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
@targos
2018-11-02, Version 11.1.0 (Current)
af6d262 
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
@targos
2018-11-02, Version 11.1.0 (Current)
74ba482 
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
@beevelop beevelop mentioned this pull request Nov 2, 2018
Closed
@Zirro Zirro mentioned this pull request Nov 2, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
@marksamman marksamman mentioned this pull request Nov 6, 2018
Closed
@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

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@ChALkeR ChALkeR Awaiting requested review from ChALkeR

@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clearTimeout blocks the process (100% CPU usage)