async_wrap: add provider types for net server #17157

AndreasMadsen · 2017-11-20T18:43:30Z

Adds TCPSERVERWRAP and PIPESERVERWRAP as async_wrap types. This makes it possible to distinguish servers from connections.

edit: not quite sure if this is semver-minor or semver-major.

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)

async_wrap, async_hooks

AndreasMadsen · 2017-11-20T18:44:26Z

/cc @nodejs/async_hooks

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

jasnell · 2017-11-20T23:16:08Z

lib/net.js

-  if (type === 'PIPE') return new Pipe();
-  if (type === 'TCP') return new TCP();
+  if (type === 'PIPE') return new Pipe(is_server);
+  if (type === 'TCP') return new TCP(is_server);


Rather than using a boolean, I'd much prefer an enum type value, similar to what we have with Http2Session, which either has type NGHTTP2_SESSION_SERVER (value 0) or NGHTTP2_SESSION_CLIENT (value 1).

Sure, I will take a look :)

AndreasMadsen · 2017-11-21T13:01:07Z

@jasnell It now uses named constants. Could you take a look again?

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

AndreasMadsen · 2017-11-23T11:15:42Z

Hehe. I was about to merge this but I will wait for the review from @addaleax, that is fine :)

AndreasMadsen · 2017-11-23T14:29:02Z

lib/net.js

@@ -1280,15 +1292,15 @@ function createServerHandle(address, port, addressType, fd) {
    handle.writable = true;
    assert(!address && !port);
  } else if (port === -1 && addressType === -1) {
-    handle = new Pipe();
+    handle = new Pipe(true);


Well, for sure this is wrong, but true is cast to 1 which is why it works :D

addaleax · 2017-11-23T14:31:58Z

src/tcp_wrap.cc

@@ -131,14 +139,28 @@ void TCPWrap::New(const FunctionCallbackInfo<Value>& args) {
  // normal function.
  CHECK(args.IsConstructCall());
  Environment* env = Environment::GetCurrent(args);
-  new TCPWrap(env, args.This());
+
+  int type_value = args[0]->IntegerValue(env->context()).ToChecked();


If you want to avoid confusion about the argument type:

CHECK(args[0]->IsInt32()); int type_value = args[0].As<Int32>()->Value();

:)

(I prefer that style in general, btw.)

Good suggestion :)

Adds `TCPSERVERWRAP` and `PIPESERVERWRAP` as provider types. This makes it possible to distinguish servers from connections.

AndreasMadsen · 2017-11-23T16:14:30Z

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

AndreasMadsen · 2017-11-24T12:55:24Z

Apparently, there was a large number of places were the type constant was missing. Please review again, as the last commit became quite large.

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

AndreasMadsen · 2017-11-24T13:50:42Z

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

addaleax · 2017-11-28T01:58:08Z

Landed in b44efde

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: #17631

Adds `TCPSERVERWRAP` and `PIPESERVERWRAP` as provider types. This makes it possible to distinguish servers from connections. PR-URL: #17157 Backport-PR-URL: #17623 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Adds `TCPSERVERWRAP` and `PIPESERVERWRAP` as provider types. This makes it possible to distinguish servers from connections. PR-URL: nodejs#17157 Backport-PR-URL: nodejs#18179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Adds `TCPSERVERWRAP` and `PIPESERVERWRAP` as provider types. This makes it possible to distinguish servers from connections. Backport-PR-URL: #18179 PR-URL: #17157 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

A new argument is necessary to the TCPWrap constructor due to nodejs/node#17157

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

Correct async hooks resource names to match the implementation: `FSREQWRAP` => `FSREQCALLBACK` `TCPSERVER` => `TCPSERVERWRAP` PR-URL: nodejs#24001 Refs: nodejs#21971 Refs: nodejs#17157 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Correct async hooks resource names to match the implementation: `FSREQWRAP` => `FSREQCALLBACK` `TCPSERVER` => `TCPSERVERWRAP` PR-URL: #24001 Refs: #21971 Refs: #17157 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Correct async hooks resource names to match the implementation: `TCPSERVER` => `TCPSERVERWRAP` Refs: #17157 PR-URL: #24684 Refs: #17157 Refs: #24001 Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

Correct async hooks resource names to match the implementation: `TCPSERVER` => `TCPSERVERWRAP` Backport-PR-URL: #24683 PR-URL: #24001 Refs: #21971 Refs: #17157 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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. labels Nov 20, 2017

AndreasMadsen added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 20, 2017

jasnell reviewed Nov 20, 2017

View reviewed changes

jasnell approved these changes Nov 21, 2017

View reviewed changes

jasnell requested a review from addaleax November 22, 2017 16:29

addaleax approved these changes Nov 23, 2017

View reviewed changes

AndreasMadsen commented Nov 23, 2017

View reviewed changes

addaleax reviewed Nov 23, 2017

View reviewed changes

AndreasMadsen added 3 commits November 23, 2017 16:55

async_wrap: add provider types for net server

b64a769

Adds `TCPSERVERWRAP` and `PIPESERVERWRAP` as provider types. This makes it possible to distinguish servers from connections.

[squash] use constants

f28cfdc

[squash] type check input

537c62d

[squash] add missing types

956124d

AndreasMadsen requested review from addaleax and jasnell November 24, 2017 12:54

[squash] fix benchmark

109feb9

addaleax approved these changes Nov 28, 2017

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 28, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

This was referenced Jan 16, 2018

Semver Minor Tracking nodejs/Release#298

Closed

Backport async hooks to v8.x #18179

Closed

jhiesey added a commit to jhiesey/pull-net that referenced this pull request Jan 31, 2018

fix exception on node 9

2729473

A new argument is necessary to the TCPWrap constructor due to nodejs/node#17157

jhiesey mentioned this pull request Jan 31, 2018

fix exception on node 9 dominictarr/pull-net#5

Merged

BethGriggs added backported-to-v8.x and removed backport-requested-v8.x labels Jun 20, 2018

Flarna mentioned this pull request Oct 31, 2018

doc: correct async_hooks resource names #24001

Closed

2 tasks

This was referenced Nov 27, 2018

[v10.x backport] doc: correct async_hooks resource names #24683

Closed

[v8.x backport] doc: correct async_hooks resource names #24684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_wrap: add provider types for net server #17157

async_wrap: add provider types for net server #17157

AndreasMadsen commented Nov 20, 2017 •

edited

Loading

AndreasMadsen commented Nov 20, 2017

jasnell Nov 20, 2017

AndreasMadsen Nov 21, 2017

AndreasMadsen commented Nov 21, 2017

AndreasMadsen commented Nov 23, 2017 •

edited

Loading

AndreasMadsen Nov 23, 2017 •

edited

Loading

addaleax Nov 23, 2017

AndreasMadsen Nov 23, 2017

AndreasMadsen commented Nov 23, 2017

AndreasMadsen commented Nov 24, 2017 •

edited

Loading

AndreasMadsen commented Nov 24, 2017 •

edited

Loading

addaleax commented Nov 28, 2017

async_wrap: add provider types for net server #17157

async_wrap: add provider types for net server #17157

Conversation

AndreasMadsen commented Nov 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

AndreasMadsen commented Nov 20, 2017

jasnell Nov 20, 2017

Choose a reason for hiding this comment

AndreasMadsen Nov 21, 2017

Choose a reason for hiding this comment

AndreasMadsen commented Nov 21, 2017

AndreasMadsen commented Nov 23, 2017 • edited Loading

AndreasMadsen Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax Nov 23, 2017

Choose a reason for hiding this comment

AndreasMadsen Nov 23, 2017

Choose a reason for hiding this comment

AndreasMadsen commented Nov 23, 2017

AndreasMadsen commented Nov 24, 2017 • edited Loading

AndreasMadsen commented Nov 24, 2017 • edited Loading

addaleax commented Nov 28, 2017

AndreasMadsen commented Nov 20, 2017 •

edited

Loading

AndreasMadsen commented Nov 23, 2017 •

edited

Loading

AndreasMadsen Nov 23, 2017 •

edited

Loading

AndreasMadsen commented Nov 24, 2017 •

edited

Loading

AndreasMadsen commented Nov 24, 2017 •

edited

Loading