[8.x] Backport 18513, 18694 #19517

ofrobots · 2018-03-21T16:21:28Z

8.x backport of #18513, #18694. @nodejs/lts this probably requires extra scrutiny as it backports a semver-minor deprecation – because async_hooks is experimental – to an LTS release.

I don't have too strong feelings that the deprecation must be backported to 8.x, but the rest of the commit (addition of the runInAsyncScope API) would be good to backport so that users have a consistent async_hooks API available across node versions. I'd be open to dropping the deprecation, if people feel strongly.

There are very few, but non-zero, users of the async hooks embedder API at this point.

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

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

MylesBorins · 2018-03-21T16:34:27Z

@ofrobots if the api is experimental aren't any changes considered semver-patch?

ofrobots · 2018-03-27T00:14:33Z

@MylesBorins that's a good point. I'm fine with whatever @nodejs/lts decides on this.

ofrobots · 2018-04-20T20:40:25Z

ping @nodejs/lts, how do we proceed with this one.

I have rebased this PR. New CI: https://ci.nodejs.org/job/node-test-pull-request/14410/.

MylesBorins · 2018-05-01T21:13:58Z

@ofrobots we can plan a semver-minor release for the next cycle of 8.x. For now I think this has to wait

MylesBorins · 2018-05-22T15:52:05Z

@ofrobots can we please get a rebase and another kick off of CI to see if those failures are related

ofrobots · 2018-05-22T23:57:08Z

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

MylesBorins · 2018-05-23T16:07:53Z

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

ofrobots · 2018-06-14T18:30:28Z

Ping @nodejs/async_hooks @nodejs/diagnostics in case people have opinions on this backport given discussion about async-hooks being "quasi" experimental at the Diag WG meeting yesterday.

~~@nodejs/lts this has been open for a while.~~ Edited, didn't intend this to be impolite.. let me know if I can / should help land this.

MylesBorins · 2018-06-14T18:32:41Z

One more CI, if green I'll land

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

Next 8.x is going to be semver minor

mcollina

LGTM without the deprecation.
(side note, we should remove those from master).

ofrobots · 2018-06-14T18:48:19Z

@mcollina can you clarify? You want to remove the deprecation from master? Those have long shipped as part of Node 10.

mcollina · 2018-06-14T18:55:09Z

I mean remove the deprecated methods from master.

MylesBorins · 2018-06-14T20:21:56Z

@mcollina are you asking for code changes to remove the deprecation in this pr?

mcollina · 2018-06-14T20:37:12Z

I’d prefer to, but I won’t block this if the prevailing opinion is to deprecate in 8.

MylesBorins · 2018-06-20T15:05:41Z

@ofrobots this needs a rebase, and could you respond to @mcollina's comment

ofrobots · 2018-06-20T17:45:58Z

Rebased.

Removed runtime deprecation warnings, so this is a docs-only deprecation for the purposes of the back-port to 8.x.

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

ofrobots · 2018-06-20T18:50:54Z

Failures in the CI look unrelated. The only source change left in this PR is an addition of a method to the AsyncResource class which should not cause those http2 and timer tests to fail. Flakes?

ofrobots · 2018-06-22T22:25:24Z

@MylesBorins this should be good to go from my side.

The emit{Before,After} APIs in AsyncResource are problematic. * emit{Before,After} are named to suggest that the only thing they do is emit the before and after hooks. However, they in fact, mutate the current execution context. * They must be properly nested. Failure to do so by user code leads to catastrophic (unrecoverable) exceptions. It is very easy for the users to forget that they must be using a try/finally block around the code that must be surrounded by these operations. Even the example provided in the official docs makes this mistake. Failing to use a finally can lead to a catastrophic crash if the callback ends up throwing. This change provides a safer `runInAsyncScope` API as an alternative and deprecates emit{Before,After}. Backport-PR-URL: nodejs#19517 PR-URL: nodejs#18513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Refs: nodejs#18513 Backport-PR-URL: nodejs#19517 PR-URL: nodejs#18694 Refs: nodejs#18513 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

As part of the backport to 8.x, this change removes the runtime deprecation making this a docs-only deprecation for 8.x. PR-URL: nodejs#19517 Reviewed-By: mcollina - Matteo Collina <matteo.collina@gmail.com>

ofrobots · 2018-06-28T20:31:33Z

Landed on v8.x-staging as 646445b...6696271.

The emit{Before,After} APIs in AsyncResource are problematic. * emit{Before,After} are named to suggest that the only thing they do is emit the before and after hooks. However, they in fact, mutate the current execution context. * They must be properly nested. Failure to do so by user code leads to catastrophic (unrecoverable) exceptions. It is very easy for the users to forget that they must be using a try/finally block around the code that must be surrounded by these operations. Even the example provided in the official docs makes this mistake. Failing to use a finally can lead to a catastrophic crash if the callback ends up throwing. This change provides a safer `runInAsyncScope` API as an alternative and deprecates emit{Before,After}. Backport-PR-URL: #19517 PR-URL: #18513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Refs: #18513 Backport-PR-URL: #19517 PR-URL: #18694 Refs: #18513 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

As part of the backport to 8.x, this change removes the runtime deprecation making this a docs-only deprecation for 8.x. PR-URL: #19517 Reviewed-By: mcollina - Matteo Collina <matteo.collina@gmail.com>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. v8.x labels Mar 21, 2018

ofrobots added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 21, 2018

gibfahn self-assigned this Mar 22, 2018

MylesBorins force-pushed the v8.x-staging branch 2 times, most recently from 44cb0d3 to 16bf5fe Compare March 30, 2018 03:28

ofrobots force-pushed the backport-8-18513 branch from 711fb31 to fa29ef0 Compare April 20, 2018 20:39

MylesBorins force-pushed the v8.x-staging branch 2 times, most recently from 5ac4bac to 591812f Compare May 22, 2018 15:31

ofrobots force-pushed the backport-8-18513 branch from fa29ef0 to 254d8e7 Compare May 22, 2018 23:43

mcollina approved these changes Jun 14, 2018

View reviewed changes

MylesBorins force-pushed the v8.x-staging branch from 084ef60 to 859dc64 Compare June 14, 2018 20:27

ofrobots force-pushed the backport-8-18513 branch from 254d8e7 to bb3732f Compare June 20, 2018 17:34

ofrobots and others added 3 commits June 28, 2018 13:21

async_hooks: remove runtime deprecation

6696271

As part of the backport to 8.x, this change removes the runtime deprecation making this a docs-only deprecation for 8.x. PR-URL: nodejs#19517 Reviewed-By: mcollina - Matteo Collina <matteo.collina@gmail.com>

ofrobots force-pushed the backport-8-18513 branch from 0e71bc0 to 6696271 Compare June 28, 2018 20:28

ofrobots merged commit 6696271 into nodejs:v8.x-staging Jun 28, 2018

ofrobots deleted the backport-8-18513 branch June 28, 2018 20:31

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Trott mentioned this pull request Oct 2, 2020

doc: fix deprecation history #35455

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[8.x] Backport 18513, 18694 #19517

[8.x] Backport 18513, 18694 #19517

ofrobots commented Mar 21, 2018 •

edited

Loading

MylesBorins commented Mar 21, 2018

ofrobots commented Mar 27, 2018

ofrobots commented Apr 20, 2018

MylesBorins commented May 1, 2018

MylesBorins commented May 22, 2018

ofrobots commented May 22, 2018

MylesBorins commented May 23, 2018

ofrobots commented Jun 14, 2018 •

edited

Loading

MylesBorins commented Jun 14, 2018

mcollina left a comment

ofrobots commented Jun 14, 2018

mcollina commented Jun 14, 2018

MylesBorins commented Jun 14, 2018

mcollina commented Jun 14, 2018

MylesBorins commented Jun 20, 2018

ofrobots commented Jun 20, 2018

ofrobots commented Jun 20, 2018

ofrobots commented Jun 22, 2018

ofrobots commented Jun 28, 2018

[8.x] Backport 18513, 18694 #19517

[8.x] Backport 18513, 18694 #19517

Conversation

ofrobots commented Mar 21, 2018 • edited Loading

Checklist

MylesBorins commented Mar 21, 2018

ofrobots commented Mar 27, 2018

ofrobots commented Apr 20, 2018

MylesBorins commented May 1, 2018

MylesBorins commented May 22, 2018

ofrobots commented May 22, 2018

MylesBorins commented May 23, 2018

ofrobots commented Jun 14, 2018 • edited Loading

MylesBorins commented Jun 14, 2018

mcollina left a comment

Choose a reason for hiding this comment

ofrobots commented Jun 14, 2018

mcollina commented Jun 14, 2018

MylesBorins commented Jun 14, 2018

mcollina commented Jun 14, 2018

MylesBorins commented Jun 20, 2018

ofrobots commented Jun 20, 2018

ofrobots commented Jun 20, 2018

ofrobots commented Jun 22, 2018

ofrobots commented Jun 28, 2018

ofrobots commented Mar 21, 2018 •

edited

Loading

ofrobots commented Jun 14, 2018 •

edited

Loading