feat(ext/net): Add Conn.setNoDelay and Conn.setKeepAlive #13103

yos1p · 2021-12-16T02:44:28Z

Implement setNoDelay function for Deno

Closes #13057
Closes #13543

CLAassistant · 2021-12-16T02:44:32Z

All committers have signed the CLA.

yos1p · 2021-12-16T02:48:10Z

@bnoordhuis Can I ask your opinion on this implementation? Any suggestions or changes to make? Thanks!

bnoordhuis

Thanks for the PR. Left some comments but mostly LGTM.

Disabling Nagle isn't directly observable so there isn't a good way to verify it's actually working but there should at least be a test that verifies the API method exists.

ext/net/01_net.js

ext/net/lib.deno_net.d.ts

AaronO

It's best to best opcall args directly without any wrapping when possible (both for performance and simplicity). Given op_set_nodelay takes 2 args (rid, true/false) there's no need to wrap that in a composite struct since opcalls have a user-available arity of 2.

I added suggestions of the required changes below that you can commit

ext/net/ops.rs

ext/net/01_net.js

bnoordhuis

LGTM, thanks! I'll dismiss Aaron's review, his feedback has been addressed.

obsolete

bnoordhuis · 2021-12-18T10:24:57Z

This still needs a test, by the way.

bartlomieju

Are we sure this should be a stable API from the get go?

yos1p · 2021-12-22T17:49:45Z

Are we sure this should be a stable API from the get go?

I'm not sure what should qualify for this to be included in the stable API. It's true that currently I don't think we can write a test for this since we need to get TcpStream from the resource table.

bartlomieju · 2021-12-22T18:57:33Z

I don't think we can write a test for this since we need to get TcpStream from the resource table.

What is the problem with getting TcpStream from the resource table?

yos1p · 2021-12-23T04:12:19Z

What is the problem with getting TcpStream from the resource table?

The idea that I have for the unit test is to call Deno.connect, get the TcpStream, and assert that nodelay and keepalive are disabled. Then, call op_set_nodelay and op_set_keepalive function and assert that nodelay and keepalive is enabled.

But I don't know how to call that in test environment. It's a combination of calling Deno code, and get the TcpStream via Rust.

yos1p · 2021-12-24T12:42:06Z

The unit test implementation for the op_set_nodelay and op_set_keepalive is a little bit much. Not sure what you guys think. Please advise if it's necessary or how to make it more simple. I'm open to suggestions.

@bnoordhuis @bartlomieju @AaronO

bartlomieju · 2021-12-27T11:55:13Z

What is the problem with getting TcpStream from the resource table?

The idea that I have for the unit test is to call Deno.connect, get the TcpStream, and assert that nodelay and keepalive are disabled. Then, call op_set_nodelay and op_set_keepalive function and assert that nodelay and keepalive is enabled.

But I don't know how to call that in test environment. It's a combination of calling Deno code, and get the TcpStream via Rust.

Ah I see what you mean here. Indeed this is rather involved to check. Let me think about it and I'll get back to you.

lucasfcosta · 2021-12-27T17:30:31Z

cli/tests/unit/net_test.ts

+  assertEquals(3, buf[2]);
+  assert(conn.rid > 0);
+
+  assert(readResult !== null);


Considering that:

L262 already checks whether readResult equals 3

readResult is a const and assigned a primitive

Do we need this assertion or is it redundant?

lucasfcosta · 2021-12-27T17:33:20Z

cli/tests/unit/net_test.ts

+  assertEquals(3, buf[2]);
+  assert(conn.rid > 0);
+
+  assert(readResult !== null);


Same here, since readResult is a const to which a primitive gets assigned and whose value we assert upon on L298, isn't this assertion redundant?

Apologies if I'm missing anything here (this is also probably just a nitpick anyway, so I'm sorry).

These methods have no directly observable side effects and should therefore be safe to turn into no-ops. Doing so unblocks a number of third-party modules. Can be implemented once denoland/deno#13103 lands.

bartlomieju

LGTM, thank you @yos1p!

yos1p added 3 commits December 16, 2021 09:23

implement op_set_nodelay for ext/net

14968b1

Add setNoDelay fn in d.ts

59f86b3

Fix op_set_nodelay implementation

d1f3d8e

bnoordhuis reviewed Dec 16, 2021

View reviewed changes

ext/net/01_net.js Outdated Show resolved Hide resolved

ext/net/lib.deno_net.d.ts Outdated Show resolved Hide resolved

AaronO previously requested changes Dec 16, 2021

View reviewed changes

ext/net/ops.rs Outdated Show resolved Hide resolved

ext/net/ops.rs Outdated Show resolved Hide resolved

ext/net/01_net.js Outdated Show resolved Hide resolved

yos1p added 2 commits December 18, 2021 14:15

Change op_set_no_delay to sync

d6c7f41

Default parameter of setNoDelay to true

42f34dc

bnoordhuis approved these changes Dec 18, 2021

View reviewed changes

bnoordhuis requested a review from AaronO December 18, 2021 10:22

yos1p marked this pull request as ready for review December 18, 2021 10:22

yos1p requested review from bartlomieju, crowlKats and lucacasonato as code owners December 18, 2021 10:22

Fix setNoDelay and add some test

ade7423

bartlomieju reviewed Dec 20, 2021

View reviewed changes

yos1p added 2 commits December 22, 2021 16:15

Prototype test for tcp setnodelay

edbea11

rollback test

8c57963

yos1p added 2 commits December 23, 2021 10:00

Add set_keepalive & socket2 dependency

5e04c43

Use closure to set socket options

e80a6bd

yos1p changed the title ~~feat(ext/net) add op_set_nodelay~~ feat(ext/net) add op_set_nodelay and op_set_keepalive Dec 23, 2021

yos1p added 3 commits December 24, 2021 16:37

Add ops test for set_nodelay and set_keepalive

00f422a

fix import in test

843890c

Use main server as tcp test

2b3dbe6

yos1p added 2 commits December 24, 2021 18:23

Fix addr in use issue in test

2ed4fdd

fix test

5f7cf60

lucasfcosta reviewed Dec 27, 2021

View reviewed changes

bnoordhuis mentioned this pull request Jan 17, 2022

fix(node/net): setNoDelay and setKeepAlive no-ops denoland/std#1828

Merged

yos1p requested a review from bartlomieju January 30, 2022 01:12

Merge branch 'main' into set_nodelay

bc2efe2

yos1p requested a review from littledivy as a code owner January 31, 2022 15:14

bartlomieju approved these changes Jan 31, 2022

View reviewed changes

bartlomieju changed the title ~~feat(ext/net) add op_set_nodelay and op_set_keepalive~~ feat(ext/net): Add Conn.setNoDelay and Conn.setKeepAlive Jan 31, 2022

bartlomieju mentioned this pull request Jan 31, 2022

Support SO_REUSEADDR #1195

Closed

bartlomieju merged commit 3e566bb into denoland:main Jan 31, 2022

yos1p deleted the set_nodelay branch January 31, 2022 15:47

kt3k mentioned this pull request Feb 1, 2022

chore: update mock Deno.Conn denoland/std#1867

Merged

Clemens-git76 mentioned this pull request May 13, 2024

[Snyk] Security upgrade jest from 27.5.1 to 29.0.0 Clemens-git76/deno#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ext/net): Add Conn.setNoDelay and Conn.setKeepAlive #13103

feat(ext/net): Add Conn.setNoDelay and Conn.setKeepAlive #13103

yos1p commented Dec 16, 2021 •

edited by bartlomieju

Loading

CLAassistant commented Dec 16, 2021 •

edited

Loading

yos1p commented Dec 16, 2021

bnoordhuis left a comment

AaronO left a comment

bnoordhuis left a comment

bnoordhuis commented Dec 18, 2021

bartlomieju left a comment

yos1p commented Dec 22, 2021

bartlomieju commented Dec 22, 2021

yos1p commented Dec 23, 2021

yos1p commented Dec 24, 2021

bartlomieju commented Dec 27, 2021

lucasfcosta Dec 27, 2021 •

edited

Loading

lucasfcosta Dec 27, 2021

bartlomieju left a comment

feat(ext/net): Add Conn.setNoDelay and Conn.setKeepAlive #13103

feat(ext/net): Add Conn.setNoDelay and Conn.setKeepAlive #13103

Conversation

yos1p commented Dec 16, 2021 • edited by bartlomieju Loading

CLAassistant commented Dec 16, 2021 • edited Loading

yos1p commented Dec 16, 2021

bnoordhuis left a comment

Choose a reason for hiding this comment

AaronO left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis commented Dec 18, 2021

bartlomieju left a comment

Choose a reason for hiding this comment

yos1p commented Dec 22, 2021

bartlomieju commented Dec 22, 2021

yos1p commented Dec 23, 2021

yos1p commented Dec 24, 2021

bartlomieju commented Dec 27, 2021

lucasfcosta Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

lucasfcosta Dec 27, 2021

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

yos1p commented Dec 16, 2021 •

edited by bartlomieju

Loading

CLAassistant commented Dec 16, 2021 •

edited

Loading

lucasfcosta Dec 27, 2021 •

edited

Loading