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

src: break out of timers loop if !can_call_into_js() #20884

Closed
wants to merge 1 commit into from
Closed

src: break out of timers loop if !can_call_into_js() #20884

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Split out from #20876

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels May 22, 2018
benjamingr
benjamingr approved these changes May 22, 2018
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I think it's probably worth expanding the commit message a little - LGTM

@addaleax
Copy link
Member Author

Expanded the commit message a bit – if you’re looking for something in particular, let me know :)

@addaleax
src: break out of timers loop if !can_call_into_js()
5f84afe 
Otherwise, this turns into an infinite loop when the flag
is set, because it makes `MakeCallback()` return immediately.
@addaleax
Copy link
Member Author

Rebased, new CI: https://ci.nodejs.org/job/node-test-commit/18687/

@benjamingr benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 22, 2018
@apapirovski
Copy link
Member

Landed in 694ac6d

apapirovski pushed a commit that referenced this pull request May 31, 2018
@addaleax @apapirovski
src: break out of timers loop if !can_call_into_js()
694ac6d 
Otherwise, this turns into an infinite loop when the flag
is set, because it makes `MakeCallback()` return immediately.

PR-URL: #20884
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins
Copy link
Contributor

@nodejs/fs and @addaleax Should this be backported to 10.x? It seems like perhaps it is dependent on a Semver-Major and should be skipped.

@addaleax addaleax deleted the timers-loop-break-out branch June 6, 2018 09:19
addaleax added a commit to addaleax/node that referenced this pull request Jun 6, 2018
@addaleax
src: break out of timers loop if !can_call_into_js()
2c58658 
Otherwise, this turns into an infinite loop when the flag
is set, because it makes `MakeCallback()` return immediately.

PR-URL: nodejs#20884
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@addaleax addaleax mentioned this pull request Jun 6, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Jun 6, 2018
@addaleax @MylesBorins
src: break out of timers loop if !can_call_into_js()
afc811c 
Otherwise, this turns into an infinite loop when the flag
is set, because it makes `MakeCallback()` return immediately.

Backport-PR-URL: #21168
PR-URL: #20884
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
Merged
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.
9 participants
@addaleax @apapirovski @MylesBorins @jasnell @benjamingr @cjihrig @tniessen @targos @nodejs-github-bot