-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
net: add setTypeOfService and getTypeOfService to Socket #61503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work. Can you add the documentation?
|
Thanks @mcollina , is this ready to be merged? Please let me know if there is anything else you need me to adjust. |
42d5142 to
c0407b2
Compare
|
@mcollina I'm confused by the CI failures. Any ideas? Or just flaky? |
ac01361 to
2bb4591
Compare
|
@ronag kindly approve once again, i fixed the issue related to win vs2022 build error on my win laptop, and also i tested it here. i guess it should work, rest the rhel and linuxone test failures are flaky, i confirmed it by looking the console logs. also i added a todo for the libuv in future. as i saw it will take time for the required api in the libuv. |
|
i have fixed all issues from my side, on both linux and windows. all platforms are ready to get served with the 26 pre version. rest ci failures are flaky. Thanks! |
| if (options.tos !== undefined) { | ||
| validateInt32(options.tos, 'options.tos', 0, 255); | ||
| } | ||
| this[kSetTOS] = options.tos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ This isn't documented, is it? Should it also be called options.typeOfService instead of options.tos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have updated the net.md doc, and will roll out in the next commit. and i have kept ADDME in the version, as i dont have much knowledge about versioning, so kindly update me from your side regarding that, i am attaching a screenshot of that here and code as well
### `new net.Socket([options])`
<!-- YAML
added: v0.3.4
changes:
- version: ADDME
pr-url: https://github.com/nodejs/node/pull/61503
description: Added `typeOfService` option.
- version: v15.14.0
pr-url: https://github.com/nodejs/node/pull/37735
description: AbortSignal support was added.
- version: v12.10.0
pr-url: https://github.com/nodejs/node/pull/25436
description: Added `onread` option.
-->
* `options` {Object} Available options are:
* `allowHalfOpen` {boolean} If set to `false`, then the socket will
automatically end the writable side when the readable side ends. See
[`net.createServer()`][] and the [`'end'`][] event for details. **Default:**
`false`.
* `blockList` {net.BlockList} `blockList` can be used for disabling outbound
access to specific IP addresses, IP ranges, or IP subnets.
* `fd` {number} If specified, wrap around an existing socket with
the given file descriptor, otherwise a new socket will be created.
* `keepAlive` {boolean} If set to `true`, it enables keep-alive functionality on
the socket immediately after the connection is established, similarly on what
is done in [`socket.setKeepAlive()`][]. **Default:** `false`.
* `keepAliveInitialDelay` {number} If set to a positive number, it sets the
initial delay before the first keepalive probe is sent on an idle socket. **Default:** `0`.
* `noDelay` {boolean} If set to `true`, it disables the use of Nagle's algorithm
immediately after the socket is established. **Default:** `false`.
* `onread` {Object} If specified, incoming data is stored in a single `buffer`
and passed to the supplied `callback` when data arrives on the socket.
This will cause the streaming functionality to not provide any data.
The socket will emit events like `'error'`, `'end'`, and `'close'`
as usual. Methods like `pause()` and `resume()` will also behave as
expected.
* `buffer` {Buffer|Uint8Array|Function} Either a reusable chunk of memory to
use for storing incoming data or a function that returns such.
* `callback` {Function} This function is called for every chunk of incoming
data. Two arguments are passed to it: the number of bytes written to
`buffer` and a reference to `buffer`. Return `false` from this function to
implicitly `pause()` the socket. This function will be executed in the
global context.
* `readable` {boolean} Allow reads on the socket when an `fd` is passed,
otherwise ignored. **Default:** `false`.
* `signal` {AbortSignal} An Abort signal that may be used to destroy the
socket.
* `typeOfService` {number} The initial Type of Service (TOS) value.
* `writable` {boolean} Allow writes on the socket when an `fd` is passed,
otherwise ignored. **Default:** `false`.
* Returns: {net.Socket}
Creates a new socket object.
The newly created socket can be either a TCP socket or a streaming [IPC][]
endpoint, depending on what it [`connect()`][`socket.connect()`] to.| if (!this._handle) { | ||
| this[kSetTOS] = tos; | ||
| return this; | ||
| } | ||
|
|
||
| if (!this._handle.setTypeOfService) { | ||
| this[kSetTOS] = tos; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!this._handle) { | |
| this[kSetTOS] = tos; | |
| return this; | |
| } | |
| if (!this._handle.setTypeOfService) { | |
| this[kSetTOS] = tos; | |
| return this; | |
| } | |
| if (!this._handle?.setTypeOfService) { | |
| this[kSetTOS] = tos; | |
| return this; | |
| } |
| @@ -74,6 +74,8 @@ class TCPWrap : public ConnectionWrap<TCPWrap, uv_tcp_t> { | |||
| static void New(const v8::FunctionCallbackInfo<v8::Value>& args); | |||
| static void SetNoDelay(const v8::FunctionCallbackInfo<v8::Value>& args); | |||
| static void SetKeepAlive(const v8::FunctionCallbackInfo<v8::Value>& args); | |||
| static void SetTOS(const v8::FunctionCallbackInfo<v8::Value>& args); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use SetTypeOfService rather than these abbreviations please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was on the way to that, i am parallely doing the changes on my 3 machines, win, linux and mac. constantly building, linting, formatting on a single change, will roll out soon
juanarbol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think windows may need a different implementation, with a different API.
Do not use. Type of Service (TOS) settings should only be set using the Quality of Service API. See Differentiated Services in the Quality of Service section of the Platform SDK for more information.
https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options
|
|
||
| // 3. Perform the system call (Platform specific casting) | ||
| #ifdef _WIN32 | ||
| if (setsockopt(reinterpret_cast<::SOCKET>(fd), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, windows is not gonna be very happy about this
Do not use. Type of Service (TOS) settings should only be set using the Quality of Service API. See Differentiated Services in the Quality of Service section of the Platform SDK for more information.
Refs: https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we should skip windows with an UV_ENOSYS, function not implemented on windows?? as i guess its libuv's job, and have you started working on that??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That method works exactly as you expect. The warning is about API design and future-proofing, not that the socket option is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue it's fine to use for now.
|
so is everything fine and ready to merge??? |
this PR implements
socket.setTOS(tos)andsocket.getTOS()innet.Socket. it needed this to support DSCP tagging (QoS) for traffic prioritization, which wasn't previously exposed in the JS API. for the implementation:tcp_wrap.ccto attemptIP_TOSfirst, and fallback toIPV6_TCLASSif that fails. This handles both IPv4 and IPv6 sockets automatically without needing a separate flag.UV_ENOSYSfor now since the headers/implementation differ there.test-net-socket-tos.js) to verify input validation and ensure the values are actually set on supported platforms.Fixes: #61489