Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[v9.x backport] 17738 and 17841 #18488

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
apapirovski wants to merge 2 commits into from
Closed

[v9.x backport] 17738 and 17841 #18488

apapirovski wants to merge 2 commits into from

Conversation

@apapirovski
Copy link
Contributor

@apapirovski apapirovski commented Jan 31, 2018 •
edited
Loading

Just required commits applied in the right order.

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

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

process

@apapirovski
process: refactor nextTick for clarity
5257623 
Do not share unnecessary information about nextTick state
between JS & C++, instead only track whether a nextTick
is scheduled or not.

Turn nextTickQueue into an Object instead of a class
since multiple instances are never created.

Other assorted refinements and refactoring.

PR-URL: nodejs#17738
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@apapirovski
process: do not directly schedule _tickCallback in _fatalException
b416a7b 
When a process encounters a _fatalException that is caught, it should
schedule execution of nextTicks but not in an arbitrary place of the
next Immediates queue. Instead, add a no-op function to the queue
that will ensure processImmediate runs, which will then ensure
that nextTicks are processed at the end.

PR-URL: nodejs#17841
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@apapirovski apapirovski requested a review from evanlucas January 31, 2018 20:00
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Jan 31, 2018
@apapirovski
Copy link
Contributor Author

apapirovski commented Jan 31, 2018 •
edited
Loading

Once this lands, #17736 #17881 #17879 #18139 should land and not fail the tests if applied in the right order.

@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 20, 2018

@apapirovski after landing this PR I am still seeing #17736 fail as below. Would it be possible for you to add all of those other blocked PRs to this backport PR and ensure everything is working

=== release test-timer-immediate ===
Path: parallel/test-timer-immediate
module.js:704
  process._tickCallback();
          ^

TypeError: process._tickCallback is not a function
    at Function.Module.runMain (module.js:704:11)
    at startup (bootstrap_node.js:190:16)
    at bootstrap_node.js:657:3

As an aside, #18139 was not landing cleanly

This was referenced Feb 20, 2018
Closed
Closed
@MylesBorins MylesBorins force-pushed the v9.x-staging branch 4 times, most recently from b9b5810 to 6edf952 Compare February 21, 2018 23:13
@apapirovski
Copy link
Contributor Author

apapirovski commented Feb 23, 2018 •
edited
Loading

@MylesBorins working on this now. Should have an update shortly. Just an FYI, the unref Immediates in #18139 are semver-minor so will need to wait until 9.7.0 now.

@apapirovski
Copy link
Contributor Author

apapirovski commented Feb 26, 2018

I'm going to open a new PR for this given the expanded scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@evanlucas evanlucas Awaiting requested review from evanlucas

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@apapirovski @MylesBorins @BridgeAR @nodejs-github-bot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.