[v9.x-backport] Backport #17198, #17738, #17841, #17736, #17881, #17879 and #18139 #19006

apapirovski · 2018-02-26T15:53:09Z

As per title, backports a whole bunch of inter-dependent PRs.

Note that #17198 used to be marked as semver-major but several TSC members have approved it being downgraded as it's non-breaking and is a dependency for several subsequent changes.

There is at least one semver-minor PR in here so this can't land until 9.7.0 at the earliest.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Share `process` through the module wrapper rather than relying on nobody messing with `global.process`. PR-URL: nodejs#17198 Fixes: nodejs#6802 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. PR-URL: nodejs#17736 Fixes: nodejs#17681 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#17881 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

If an error is encountered during the processing of Immediates, schedule the remaining queue to finish after all error handling code runs (if the process is still alive to do so). The new changes make the Immediates error handling behaviour entirely deterministic and predictable, as the full queue will be flushed on each Immediates cycle, regardless of whether an error is encountered or not. Currently this processing is scheduled for nextTick which can yield unpredictable results as the nextTick might happen as early as close callbacks phase or as late as after the next event loop turns Immediates all fully processed. The latter can result in two full cycles of Immediates processing during one even loop turn. The current implementation also doesn't differentiate between Immediates scheduled for the current queue run or the next one, so Immediates that were scheduled for the next turn of the event loop, will process alongside the ones that were scheduled for the current turn. PR-URL: nodejs#17879 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refactor Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Document the new `immediate.ref()` and `immediate.unref()` methods. Add SetImmediateUnref on the C++ side. PR-URL: nodejs#18139 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]>

apapirovski · 2018-02-26T15:53:46Z

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

addaleax

Nice, thanks!

Share `process` through the module wrapper rather than relying on nobody messing with `global.process`. Backport-PR-URL: #19006 PR-URL: #17198 Fixes: #6802 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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. Backport-PR-URL: #19006 PR-URL: #17738 Reviewed-By: Anna Henningsen <[email protected]>

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. Backport-PR-URL: #19006 PR-URL: #17841 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. Backport-PR-URL: #19006 PR-URL: #17736 Fixes: #17681 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #19006 PR-URL: #17881 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

If an error is encountered during the processing of Immediates, schedule the remaining queue to finish after all error handling code runs (if the process is still alive to do so). The new changes make the Immediates error handling behaviour entirely deterministic and predictable, as the full queue will be flushed on each Immediates cycle, regardless of whether an error is encountered or not. Currently this processing is scheduled for nextTick which can yield unpredictable results as the nextTick might happen as early as close callbacks phase or as late as after the next event loop turns Immediates all fully processed. The latter can result in two full cycles of Immediates processing during one even loop turn. The current implementation also doesn't differentiate between Immediates scheduled for the current queue run or the next one, so Immediates that were scheduled for the next turn of the event loop, will process alongside the ones that were scheduled for the current turn. Backport-PR-URL: #19006 PR-URL: #17879 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refactor Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Document the new `immediate.ref()` and `immediate.unref()` methods. Add SetImmediateUnref on the C++ side. Backport-PR-URL: #19006 PR-URL: #18139 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax · 2018-02-26T18:00:01Z

Thanks, landed on v9.x-staging!

landed in 743cf33...f2dd17b

Share `process` through the module wrapper rather than relying on nobody messing with `global.process`. Backport-PR-URL: #19006 PR-URL: #17198 Fixes: #6802 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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. Backport-PR-URL: #19006 PR-URL: #17738 Reviewed-By: Anna Henningsen <[email protected]>

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. Backport-PR-URL: #19006 PR-URL: #17841 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. Backport-PR-URL: #19006 PR-URL: #17736 Fixes: #17681 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #19006 PR-URL: #17881 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

If an error is encountered during the processing of Immediates, schedule the remaining queue to finish after all error handling code runs (if the process is still alive to do so). The new changes make the Immediates error handling behaviour entirely deterministic and predictable, as the full queue will be flushed on each Immediates cycle, regardless of whether an error is encountered or not. Currently this processing is scheduled for nextTick which can yield unpredictable results as the nextTick might happen as early as close callbacks phase or as late as after the next event loop turns Immediates all fully processed. The latter can result in two full cycles of Immediates processing during one even loop turn. The current implementation also doesn't differentiate between Immediates scheduled for the current queue run or the next one, so Immediates that were scheduled for the next turn of the event loop, will process alongside the ones that were scheduled for the current turn. Backport-PR-URL: #19006 PR-URL: #17879 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refactor Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Document the new `immediate.ref()` and `immediate.unref()` methods. Add SetImmediateUnref on the C++ side. Backport-PR-URL: #19006 PR-URL: #18139 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]>

Share `process` through the module wrapper rather than relying on nobody messing with `global.process`. Backport-PR-URL: #19006 PR-URL: #17198 Fixes: #6802 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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. Backport-PR-URL: #19006 PR-URL: #17738 Reviewed-By: Anna Henningsen <[email protected]>

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. Backport-PR-URL: #19006 PR-URL: #17841 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. Backport-PR-URL: #19006 PR-URL: #17736 Fixes: #17681 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #19006 PR-URL: #17881 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

If an error is encountered during the processing of Immediates, schedule the remaining queue to finish after all error handling code runs (if the process is still alive to do so). The new changes make the Immediates error handling behaviour entirely deterministic and predictable, as the full queue will be flushed on each Immediates cycle, regardless of whether an error is encountered or not. Currently this processing is scheduled for nextTick which can yield unpredictable results as the nextTick might happen as early as close callbacks phase or as late as after the next event loop turns Immediates all fully processed. The latter can result in two full cycles of Immediates processing during one even loop turn. The current implementation also doesn't differentiate between Immediates scheduled for the current queue run or the next one, so Immediates that were scheduled for the next turn of the event loop, will process alongside the ones that were scheduled for the current turn. Backport-PR-URL: #19006 PR-URL: #17879 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refactor Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Document the new `immediate.ref()` and `immediate.unref()` methods. Add SetImmediateUnref on the C++ side. Backport-PR-URL: #19006 PR-URL: #18139 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-08-02T16:44:06Z

@apapirovski should we be backporting all these to v8.x?

addaleax and others added 7 commits February 26, 2018 10:25

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 Feb 26, 2018

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 26, 2018

addaleax approved these changes Feb 26, 2018

View reviewed changes

addaleax closed this Feb 26, 2018

addaleax added backported-to-v9.x and removed v9.x labels Feb 26, 2018

apapirovski deleted the backport-17738 branch April 22, 2018 10:17

MylesBorins mentioned this pull request Jul 31, 2018

lib: add process to internal module wrapper #17198

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v9.x-backport] Backport #17198, #17738, #17841, #17736, #17881, #17879 and #18139 #19006

[v9.x-backport] Backport #17198, #17738, #17841, #17736, #17881, #17879 and #18139 #19006

apapirovski commented Feb 26, 2018

apapirovski commented Feb 26, 2018

addaleax left a comment

addaleax commented Feb 26, 2018 •

edited by MylesBorins

Loading

MylesBorins commented Aug 2, 2018

[v9.x-backport] Backport #17198, #17738, #17841, #17736, #17881, #17879 and #18139 #19006

[v9.x-backport] Backport #17198, #17738, #17841, #17736, #17881, #17879 and #18139 #19006

Conversation

apapirovski commented Feb 26, 2018

Checklist

Affected core subsystem(s)

apapirovski commented Feb 26, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Feb 26, 2018 • edited by MylesBorins Loading

MylesBorins commented Aug 2, 2018

addaleax commented Feb 26, 2018 •

edited by MylesBorins

Loading