net: add new options to `net.Socket` and `net.Server` #41310

ShogunPanda · 2021-12-24T13:29:18Z

Hello!
This PR adds the new following options to net.Socket.connect:

noDelay (boolean): If set to true, calls uv_tcp_nodelay on the socket after connection. By default it is disabled.
keepAlive (boolean) and keepAliveDelay (number): If set to true calls uv_tcp_nodelay on the socket after connection. By default it is disabled.

This changes are needed as preliminary work on #34185 to avoid excessive JS to C++ bridging.
As I'm not a C++ expert, I encountered the following problems:

Not sure how to test changes (even though test suite is passing so apparently nothing is broken)
Error handling in TCPWrap::AfterConnect was tentative so some C++ expert should take a look. Moreover, what shall we do if uv_tcp_connect succeeds but any of uv_tcp_nodelay or uv_tcp_keepalive fails? Shall we close the socket or let user handle it? Currently I don't do anything.

Once this is initially reviewed, I will also add (either here or in a new PR) the analogous on inbound connections.

nodejs-github-bot · 2021-12-24T13:29:22Z

Review requested:

@nodejs/http
@nodejs/net

mcollina · 2021-12-24T14:03:33Z

How do we test those options when set via the other methods?

addaleax · 2021-12-25T18:23:10Z

This changes are needed as preliminary work on #34185 to avoid excessive JS to C++ bridging.

I would want to avoid as much additional complexity as possible -- this adds quite a bit, and even adds new members to the TCPWrap class, even though they are only useful during connecting. I would personally prefer to not do this and instead call the existing C++ methods from JS.

2. Moreover, what shall we do if uv_tcp_connect succeeds but any of uv_tcp_nodelay or uv_tcp_keepalive fails? Shall we close the socket or let user handle it?

Close the socket, yes, but also let the user handle the error itself.

addaleax · 2021-12-25T18:24:06Z

src/tcp_wrap.h

@@ -64,6 +65,10 @@ class TCPWrap : public ConnectionWrap<TCPWrap, uv_tcp_t> {
 private:
  typedef uv_tcp_t HandleType;

+  bool noDelay;
+  bool keepAlive;
+  unsigned int keepAliveDelay;


If this PR adds new fields, then they should be using lower_snake_case_; as elsewhere in the code

Sorry, I missed that. No worries, I'll fix once we agree on the final approach.

mcollina · 2021-12-25T18:37:47Z

src/tcp_wrap.cc

@@ -325,14 +347,22 @@ void TCPWrap::Connect(const FunctionCallbackInfo<Value>& args,
  T addr;
  int err = uv_ip_addr(*ip_address, &addr);

+  wrap->noDelay = args[3]->IsTrue();
+  wrap->keepAlive = args[4]->IsTrue();


Can we attach the fields to the ConnectWrap instead?

In this way it will not increase the memory footprint for each connection.

Sure, I can move them there so they are then discarded after connection.

@addaleax Any objection on using this approach? Or do you think calling JS methods will not impact performance too much?

@ShogunPanda Well yeah, @mcollina’s suggestion is definitely something that would improve this PR 🙂

Or do you think calling JS methods will not impact performance too much?

I would still think that that’s absolutely preferable, and not expect significant performance overhead, because:

These calls are performed once-per-connection, which is much less than e.g. the number of data events for typical sockets, so it will have comparably little impact, and

Doing JS-only changes would still allow us to be just as fast in the common case (by calling uv_tcp_nodelay(handle, 0); in the connect handler, and only doing another call into C++ when the user explicitly requested that the value is not set)

Generally we try to stick to implementing as much logic in JS as possible, because it is the more accessible language of the two that Node.js is made out of 🙂

ShogunPanda · 2021-12-26T20:49:36Z

@addaleax About testing, I just realized i can add two more testing in the array sent to req_wrap->MakeCallback in order to test it from the JS side. Do you think this will work?

addaleax · 2021-12-26T21:18:57Z

@addaleax About testing, I just realized i can add two more testing in the array sent to req_wrap->MakeCallback in order to test it from the JS side. Do you think this will work?

@ShogunPanda You mean, pass the values back to the callback in order to test that the values were passed along properly? I don’t think that’s necessary, and I don’t think we have other tests in that format. I think manual validation that the right syscalls are made (e.g. with strace) would be the only thing we can really do to test this here.

ShogunPanda · 2021-12-28T08:15:07Z

@ShogunPanda You mean, pass the values back to the callback in order to test that the values were passed along properly? I don’t think that’s necessary, and I don’t think we have other tests in that format. I think manual validation that the right syscalls are made (e.g. with strace) would be the only thing we can really do to test this here.

Ok, no problem at all. Will validate manually. I plan to resend this PR today for further evaluation.

ShogunPanda · 2021-12-28T15:57:53Z

@addaleax @mcollina I've rewritten the PR in JS only (as I found out JS-to-CPP calls are not introducing any significant overhead). I also added some tests.

PTAL, thanks!

lib/net.js

mcollina

lgtm

ShogunPanda · 2022-01-28T11:34:47Z

Any update on this? Shall I do anything else on this or are we good to merge?

nodejs-github-bot · 2022-01-28T16:24:51Z

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

nodejs-github-bot · 2022-02-01T11:36:28Z

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

Notable changes: * (SEMVER-MINOR) crypto: add KeyObject.prototype.equals method (Filip Skokan) nodejs#42093 * (SEMVER-MINOR) net: add new options to net.Socket and net.Server (Paolo Insogna) nodejs#41310 * (SEMVER-MINOR) src: allow preventing InitializeInspector in env (Shelley Vohr) nodejs#35025 * doc: add release key for Bryan English (Bryan English) nodejs#42102 Dependency Updates: * deps: update nghttp2 to 1.47.0 (Yash Ladha) nodejs#42127 * deps: upgrade npm to 8.5.2 (npm team) nodejs#42122 New Collaborators: * doc: add JakobJingleheimer to collaborators list (Jacob Smith) nodejs#42185 * doc: move bnoordhuis back to collaborators (Ben Noordhuis) nodejs#42064 PR-URL: nodejs#42254

Refs: nodejs#41310

Notable changes: * (SEMVER-MINOR) crypto: add KeyObject.prototype.equals method (Filip Skokan) nodejs#42093 * (SEMVER-MINOR) net: add new options to net.Socket and net.Server (Paolo Insogna) nodejs#41310 * (SEMVER-MINOR) src: allow preventing InitializeInspector in env (Shelley Vohr) nodejs#35025 * doc: add release key for Bryan English (Bryan English) nodejs#42102 Dependency Updates: * deps: update nghttp2 to 1.47.0 (Yash Ladha) nodejs#42127 * deps: upgrade npm to 8.5.2 (npm team) nodejs#42122 New Collaborators: * doc: add JakobJingleheimer to collaborators list (Jacob Smith) nodejs#42185 * doc: move bnoordhuis back to collaborators (Ben Noordhuis) nodejs#42064 PR-URL: nodejs#42254

Notable changes: * (SEMVER-MINOR) crypto: add KeyObject.prototype.equals method (Filip Skokan) #42093 * (SEMVER-MINOR) net: add new options to net.Socket and net.Server (Paolo Insogna) #41310 * (SEMVER-MINOR) src: allow preventing InitializeInspector in env (Shelley Vohr) #35025 * doc: add release key for Bryan English (Bryan English) #42102 Dependency Updates: * deps: update nghttp2 to 1.47.0 (Yash Ladha) #42127 * deps: upgrade npm to 8.5.2 (npm team) #42122 New Collaborators: * doc: add JakobJingleheimer to collaborators list (Jacob Smith) #42185 * doc: move bnoordhuis back to collaborators (Ben Noordhuis) #42064 PR-URL: #42254

Refs: nodejs#41310

Refs: #41310 PR-URL: #42268 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>