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

[v9.x backport] 17738 and 17841 #18488

Closed
wants to merge 2 commits into from
Closed

[v9.x backport] 17738 and 17841 #18488

wants to merge 2 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 31, 2018 •
edited

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>
@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
Member Author

apapirovski commented Jan 31, 2018 •
edited

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

@MylesBorins
Copy link
Contributor

@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
Member Author

apapirovski commented Feb 23, 2018 •
edited

@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
Member Author

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.

None yet
4 participants
@apapirovski @MylesBorins @BridgeAR @nodejs-github-bot