[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

mmarchini · 2018-02-03T17:08:50Z

Backport of #14901, #18530 and #18653 to v9.x.

Even though no manual changes were required while cherry-picking, I'm backporting to avoid problems with a reverted commit present in #14901 (see #14901 (comment)). I'm also backporting #18530 and #18653 since both PRs are improvements of this feature.

make -j4 test (UNIX)
tests are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src, test

MylesBorins · 2018-02-20T21:47:26Z

The original PR is labelled dont-land-on-9.x do you know why that is?

The commit that landed on master 756a34e lands cleanly on 9.x, is there a reason for the explicit backport? Is there a difference between the changes?

mmarchini · 2018-02-21T21:35:13Z

The original PR is labelled dont-land-on-9.x do you know why that is?

AFAIK it's because there was one reverted commit here, see #14901 (comment) (this comment also explains why I opened this backport PR).

mmarchini · 2018-02-22T15:41:01Z

@MylesBorins I added two related PRs to this backport, since they improve these changes.

mmarchini · 2018-02-22T15:43:40Z

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

Shutting down the connection is what `_final` is there for. PR-URL: nodejs#18608 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Currently the following deprecation warning is produced when compiling node_perf.cc: ./src/node_perf.cc:91:11: warning: 'MakeCallback' is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations] node::MakeCallback(env->isolate(), ^ ../src/node.h:172:50: note: 'MakeCallback' has been explicitly marked deprecated here NODE_EXTERN v8::Local<v8::Value> MakeCallback( ^ 1 warning generated. This commit adds an async_context to the call and checks the maybe result. PR-URL: nodejs#18877 Reviewed-By: Ali Ijaz Sheikh <[email protected]>

PR-URL: nodejs#18908 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Make sure that inspector ports in cluster are inside the valid range: `[1024, 65535]`. Fixes flaky `test-inspector-port-zero-cluster`. PR-URL: nodejs#18696 Fixes: nodejs#18303 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#18607 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#18816 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

This fixes a few rules by making sure the input is actually ready to be checked. Otherwise those can throw TypeErrors or result in faulty error messages. PR-URL: nodejs#18853 Reviewed-By: Luigi Pinca <[email protected]>

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: nodejs#16864 PR-URL: nodejs#17015 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

Set `socket._httpMessage` to `null` before emitting the `'connect'` or `'upgrade'` event. PR-URL: nodejs#18865 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs#18859 Reviewed-By: James M Snell <[email protected]>

When create a nest repl, will register `Runtime.executionContextCreated` listener to the inspector session.This patch will fix listener repeatedly register. PR-URL: nodejs#18881 Fixes: nodejs#18284 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Remove an erroneous CHECK that asserted the persistent object's internal field pointer still pointed to a valid object. If ClearWrap() has been called, the field pointer equals nullptr and that is expected behavior. PR-URL: nodejs#18898 Fixes: nodejs#18256 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>

Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

mmarchini · 2018-02-26T17:53:40Z

Commit added to avoid conflict with #18954: 5329a07cbc73d702ebf71400747530bf8ed9d6b2

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

This commit add SetUpTestCase and TearDownTestCase functions that will be called once per test case. Currently we only have SetUp/TearDown which are called for each test. This commit moves the initialization and configuration of Node and V8 to be done on a per test case basis, but gives each test a new Isolate. Backport-PR-URL: nodejs#18957 PR-URL: nodejs#18558 Reviewed-By: Ben Noordhuis <[email protected]>

Share `process` through the module wrapper rather than relying on nobody messing with `global.process`. Backport-PR-URL: nodejs#19006 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. Backport-PR-URL: nodejs#19006 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. Backport-PR-URL: nodejs#19006 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. Backport-PR-URL: nodejs#19006 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]>

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

Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 PR-URL: nodejs#14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: nodejs#18530 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. PR-URL: nodejs#18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Yihong Wang <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

mmarchini · 2018-02-26T18:14:01Z

Resolved conflict.

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

mmarchini · 2018-02-26T20:23:04Z

Another CI (just to be sure): https://ci.nodejs.org/job/node-test-pull-request/13380/

Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 Backport-PR-URL: #18550 PR-URL: #14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: #18550 PR-URL: #18530 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. Backport-PR-URL: #18550 PR-URL: #18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. Backport-PR-URL: #18550 PR-URL: #18576 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Yihong Wang <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax · 2018-02-26T22:55:48Z

I rebased a commit out of v9.x-staging that was causing failures on Windows in CI.

Landed on v9.x-staging, post-land-CI (because I’m done for tonight): https://ci.nodejs.org/job/node-test-commit/16514/

Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 Backport-PR-URL: #18550 PR-URL: #14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: #18550 PR-URL: #18530 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. Backport-PR-URL: #18550 PR-URL: #18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. Backport-PR-URL: #18550 PR-URL: #18576 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Yihong Wang <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 3, 2018

MylesBorins mentioned this pull request Feb 20, 2018

src, test: node internals' postmortem metadata #14901

Closed

2 tasks

MylesBorins force-pushed the v9.x-staging branch 3 times, most recently from 0e105ae to bc156da Compare February 21, 2018 16:07

MylesBorins force-pushed the v9.x-staging branch from b9b5810 to 6edf952 Compare February 21, 2018 23:13

mmarchini force-pushed the backport-14901-to-v9.x branch from 610fdb8 to 2c9a85d Compare February 22, 2018 15:31

mmarchini changed the title ~~[v9.x backport] src, test: node internals' postmortem metadata~~ [v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653) Feb 22, 2018

This was referenced Feb 22, 2018

src: do not redefine private for GenDebugSymbols #18653

Closed

[v9.x backport] build: add node_lib_target_name to cctest deps #18954

Closed

addaleax approved these changes Feb 26, 2018

View reviewed changes

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

addaleax and others added 12 commits February 26, 2018 17:15

net: use _final instead of on('finish')

ea86656

Shutting down the connection is what `_final` is there for. PR-URL: nodejs#18608 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

net: inline and simplify onSocketEnd

9d73910

PR-URL: nodejs#18607 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

vm: consolidate validation

ad98ff1

PR-URL: nodejs#18816 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

tools: fix custom eslint rule errors

e29acf8

This fixes a few rules by making sure the input is actually ready to be checked. Otherwise those can throw TypeErrors or result in faulty error messages. PR-URL: nodejs#18853 Reviewed-By: Luigi Pinca <[email protected]>

test: http2 compat response.write() error checks

2ad7cb9

PR-URL: nodejs#18859 Reviewed-By: James M Snell <[email protected]>

addaleax added 2 commits February 26, 2018 18:35

mmarchini force-pushed the backport-14901-to-v9.x branch from fafff2f to 5329a07 Compare February 26, 2018 17:49

danbev and others added 12 commits February 26, 2018 18:57

test: fix cctest -Wunused-variable warning

7d12ef6

PR-URL: nodejs#18530 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

mmarchini force-pushed the backport-14901-to-v9.x branch from 5329a07 to 686a66d Compare February 26, 2018 18:12

addaleax force-pushed the v9.x-staging branch from 2177138 to 319cd44 Compare February 26, 2018 22:43

addaleax closed this Feb 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

Uh oh!

mmarchini commented Feb 3, 2018 •

edited

Loading

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

mmarchini commented Feb 21, 2018

Uh oh!

mmarchini commented Feb 22, 2018

Uh oh!

mmarchini commented Feb 22, 2018

Uh oh!

mmarchini commented Feb 26, 2018

Uh oh!

mmarchini commented Feb 26, 2018

Uh oh!

mmarchini commented Feb 26, 2018

Uh oh!

addaleax commented Feb 26, 2018

Uh oh!

Uh oh!

Uh oh!

[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

Uh oh!

Conversation

mmarchini commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Affected core subsystem(s)

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

mmarchini commented Feb 21, 2018

Uh oh!

mmarchini commented Feb 22, 2018

Uh oh!

mmarchini commented Feb 22, 2018

Uh oh!

mmarchini commented Feb 26, 2018

Uh oh!

mmarchini commented Feb 26, 2018

Uh oh!

mmarchini commented Feb 26, 2018

Uh oh!

addaleax commented Feb 26, 2018

Uh oh!

Uh oh!

mmarchini commented Feb 3, 2018 •

edited

Loading