FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

timers: fix priority queue removeAt fn by apapirovski · Pull Request #23870 · nodejs/node · GitHub

/ node Public

timers: fix priority queue removeAt fn - #23870

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-priority-queue
ConversationCommits (1)ChecksFiles changed
Closed

timers: fix priority queue removeAt fn#23870
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-priority-queue

Conversation

apapirovski commented Oct 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤷‍♂️ 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 commented Oct 25, 2018

Copy link
Copy Markdown
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 commented Oct 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

targos commented Oct 25, 2018

Copy link
Copy Markdown
Member

I suppose you want to fast-track?

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

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

apapirovski commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

Please 👍 this if you approve fast tracking.

apapirovski added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 25, 2018
mcollina approved these changes Oct 25, 2018
View reviewed changes

mcollina left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott commented Oct 25, 2018

Copy link
Copy Markdown
Member

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

apapirovski commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

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

jasnell approved these changes Oct 25, 2018
View reviewed changes

jasnell left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 25, 2018

Copy link
Copy Markdown
Member

+1 to fast-track

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 25, 2018
timers: fix priority queue removeAt fn
958d5b7 
PR-URL: nodejs#23870
Fixes: nodejs#23860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Trott commented Oct 25, 2018

Copy link
Copy Markdown
Member

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 closed this Oct 25, 2018

mcollina commented Oct 25, 2018 via email

Copy link
Copy Markdown
Member

targos pushed a commit that referenced this pull request Oct 26, 2018
timers: fix priority queue removeAt fn
8f07bfb 
PR-URL: #23870
Fixes: #23860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau mentioned this pull request Oct 27, 2018
Closed
targos mentioned this pull request Oct 27, 2018
Merged
mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 27, 2018
richardlau mentioned this pull request Oct 28, 2018
Closed
targos added a commit that referenced this pull request Oct 28, 2018
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 mentioned this pull request Oct 29, 2018
Closed
targos pushed a commit that referenced this pull request Nov 1, 2018
timers: fix priority queue removeAt fn
167e99b 
PR-URL: #23870
Fixes: #23860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Nov 1, 2018
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
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 mentioned this pull request Nov 2, 2018
Closed
Zirro mentioned this pull request Nov 2, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
marksamman mentioned this pull request Nov 6, 2018
Closed

podarok commented Nov 15, 2018

Copy link
Copy Markdown

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

MylesBorins commented Nov 26, 2018

Copy link
Copy Markdown
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 commented Dec 2, 2018

Copy link
Copy Markdown

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 ;).

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
ChALkeR Awaiting requested review from ChALkeR
Fishrock123 Awaiting requested review from Fishrock123
+2 more reviewers
TimothyGu TimothyGu approved these changes
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)
Back | FazBrowse Home | New Git URL