-
-
Notifications
You must be signed in to change notification settings - Fork 34.6k
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
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? |
Commit Queue failed- Loading data for nodejs/node/pull/61503 ✔ Done loading data for nodejs/node/pull/61503 ----------------------------------- PR info ------------------------------------ Title net: add setTypeOfService and getTypeOfService to Socket (#61503) Author Amol Yadav <amyssnipet@yahoo.com> (@amyssnippet, first-time contributor) Branch amyssnippet:fix/61489 -> nodejs:main Labels c++, semver-minor, lib / src, author ready, commit-queue-squash Commits 34 - net: add setTOS and getTOS to Socket - added doc - fixed some lint issues - already complete with validation and platform specific tests - added: REPLACEME - common.mustCall used - undo: unrelated header was removed irrelevantly - fix: resolve windows compilation and linter issues - rollback to tcp only - fix: ensure getTOS returns numeric default when handle lacks getTOS - fix: handle setTOS errors in afterConnect and use options.tos - fix: capture errno immediately after getsockopt failures - fix: use correct length type for Windows getsockopt and fix DSCP mask… - fix: validate options.tos in constructor - fix: use validateInt32 for options.tos and standardize GetTOS error - fix: remove unused errno_val in TCPWrap::GetTOS POSIX path - test: add boundary value assertions for TOS - test: add pre-connect TOS caching test - doc: add notes for setTOS and getTOS about pre-connect caching and OS… - fix: import UV_EBADF and remove duplicate mask declaration - fix: update TOS cache when handle lacks setTOS - doc: clarify pre-connect TOS test comment - refined: comments - doc: remove prohibited 'Note that' phrases - fix: reorder includes for Windows compatibility and add missing POSIX… - fix: resolve windows header order and doc lint errors - feat: rename setTOS/getTOS to setTypeOfService/getTypeOfService for A… - fix: update internal handle calls to use new TypeOfService method names - win vs2022 ci fix - added: todo for libuv api - fixed:win vs2022 lint ci - feat: rename C++ methods from SetTOS/GetTOS to SetTypeOfService/GetTy… - docs: update net.md for typeOfService option and fix lib/net.js const… - Apply suggestions from code review Committers 2 - Amol Yadav <amyssnipet@yahoo.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/61503 Fixes: https://github.com/nodejs/node/issues/61489 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/61503 Fixes: https://github.com/nodejs/node/issues/61489 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 24 Jan 2026 11:11:33 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/61503#pullrequestreview-3731022920 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/61503#pullrequestreview-3730004460 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/61503#pullrequestreview-3711651185 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-01-30T23:57:17Z: https://ci.nodejs.org/job/node-test-pull-request/71128/ - Querying data for job/node-test-pull-request/71128/ ✔ Build data downloaded ✔ Last Jenkins CI successful ⚠ PR author is a new contributor: @amyssnippet(amyssnipet@yahoo.com) ⚠ - commit d53c0157fc93 is authored by ronagy@icloud.com -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/21536896438 |
|
Apparently |
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.
lgtm
|
No idea. Not familiar with that error or how to fix it. |
|
i guess @juanarbol told it here
|
|
Still not familiar with it nor do I understand how to fix it. Hopefully someone else will jump in. |
Commit Queue failed- Loading data for nodejs/node/pull/61503 ✔ Done loading data for nodejs/node/pull/61503 ----------------------------------- PR info ------------------------------------ Title net: add setTypeOfService and getTypeOfService to Socket (#61503) Author Amol Yadav <amyssnipet@yahoo.com> (@amyssnippet, first-time contributor) Branch amyssnippet:fix/61489 -> nodejs:main Labels c++, semver-minor, lib / src, author ready, commit-queue-squash Commits 34 - net: add setTOS and getTOS to Socket - added doc - fixed some lint issues - already complete with validation and platform specific tests - added: REPLACEME - common.mustCall used - undo: unrelated header was removed irrelevantly - fix: resolve windows compilation and linter issues - rollback to tcp only - fix: ensure getTOS returns numeric default when handle lacks getTOS - fix: handle setTOS errors in afterConnect and use options.tos - fix: capture errno immediately after getsockopt failures - fix: use correct length type for Windows getsockopt and fix DSCP mask… - fix: validate options.tos in constructor - fix: use validateInt32 for options.tos and standardize GetTOS error - fix: remove unused errno_val in TCPWrap::GetTOS POSIX path - test: add boundary value assertions for TOS - test: add pre-connect TOS caching test - doc: add notes for setTOS and getTOS about pre-connect caching and OS… - fix: import UV_EBADF and remove duplicate mask declaration - fix: update TOS cache when handle lacks setTOS - doc: clarify pre-connect TOS test comment - refined: comments - doc: remove prohibited 'Note that' phrases - fix: reorder includes for Windows compatibility and add missing POSIX… - fix: resolve windows header order and doc lint errors - feat: rename setTOS/getTOS to setTypeOfService/getTypeOfService for A… - fix: update internal handle calls to use new TypeOfService method names - win vs2022 ci fix - added: todo for libuv api - fixed:win vs2022 lint ci - feat: rename C++ methods from SetTOS/GetTOS to SetTypeOfService/GetTy… - docs: update net.md for typeOfService option and fix lib/net.js const… - Apply suggestions from code review Committers 2 - Amol Yadav <amyssnipet@yahoo.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/61503 Fixes: https://github.com/nodejs/node/issues/61489 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/61503 Fixes: https://github.com/nodejs/node/issues/61489 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 24 Jan 2026 11:11:33 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/61503#pullrequestreview-3736689951 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/61503#pullrequestreview-3737494805 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/61503#pullrequestreview-3711651185 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-01-31T02:05:35Z: https://ci.nodejs.org/job/node-test-pull-request/71128/ - Querying data for job/node-test-pull-request/71128/ ✔ Build data downloaded ✔ Last Jenkins CI successful ⚠ PR author is a new contributor: @amyssnippet(amyssnipet@yahoo.com) ⚠ - commit d53c0157fc93 is authored by ronagy@icloud.com -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/21587645333 |
|
@aduh95 same failure as before " PR author is a new contributor: @amyssnippet(amyssnipet@yahoo.com)" |
|
it looks like the commit queue is still failing because of mixed authorship and i am a new contributor. could anyone please manually land this PR as the bot seems unable to handle the squash in this scenario |
|
Landed in 5212c07 |
|
thanks every maintainer here to help me contribute to the nodejs community |
Notable changes: async_hooks: * (SEMVER-MINOR) add `trackPromises` option to `createHook()` (Joyee Cheung) #61415 net: * (SEMVER-MINOR) add `setTOS` and `getTOS` to `Socket` (Amol Yadav) #61503 src: * (SEMVER-MINOR) add initial support for ESM in embedder API (Joyee Cheung) #61548 * improve `TextEncoder` encode performance with `simdutf` (Mert Can Altin) #61496 stream: * (SEMVER-MINOR) add `bytes()` method to `node:stream/consumers` (wantaek) #60426 test_runner: * (SEMVER-MINOR) add `env` option to `run` function (Ethan Arrowood) #61367 url: * update Ada to v3.4.2 and support Unicode 17 (Yagiz Nizipli) #61593 PR-URL: #61635
Notable changes: async_hooks: * (SEMVER-MINOR) add `trackPromises` option to `createHook()` (Joyee Cheung) #61415 net: * (SEMVER-MINOR) add `setTOS` and `getTOS` to `Socket` (Amol Yadav) #61503 src: * (SEMVER-MINOR) add initial support for ESM in embedder API (Joyee Cheung) #61548 * improve `TextEncoder` encode performance with `simdutf` (Mert Can Altin) #61496 stream: * (SEMVER-MINOR) add `bytes()` method to `node:stream/consumers` (wantaek) #60426 test_runner: * (SEMVER-MINOR) add `env` option to `run` function (Ethan Arrowood) #61367 url: * update Ada to v3.4.2 and support Unicode 17 (Yagiz Nizipli) #61593 PR-URL: #61635
this PR implements
socket.setTypeOfService(tos)andsocket.getTypeOfService()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