net: support AF_UNIX paths in net.BoundSocket#64399
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
Signed-off-by: Guy Bedford <guybedford@gmail.com>
guybedford
force-pushed
the
net-boundsocket-pipe
branch
from
1e91371 to
6a4fee8
Compare
Only trust the adopted handle's type when it came from a BoundSocket; a TLSSocket's _handle is a TLSWrap, not a Pipe, so TLS/HTTPS over pipes must still infer pipe-ness from the path option.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64399 +/- ##
==========================================
- Coverage 90.25% 90.24% -0.01%
==========================================
Files 741 741
Lines 241207 241293 +86
Branches 45430 45448 +18
==========================================
+ Hits 217698 217754 +56
- Misses 15084 15127 +43
+ Partials 8425 8412 -13
🚀 New features to boost your workflow:
|
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
LGTM with just some doc changes and one question about the implementation.
| ### `new net.BoundSocket([options])` | ||
|
|
||
| <!-- YAML | ||
| added: v26.4.0 |
There was a problem hiding this comment.
I think you should add a changes: section here
| changes: | |
| - version: REPLACEME | |
| pr-url: https://github.com/nodejs/node/pull/64399 | |
| description: The `path` option is supported. |
| ### `boundSocket.address()` | ||
|
|
||
| <!-- YAML | ||
| added: v26.4.0 |
There was a problem hiding this comment.
Need a changes: line here too I think.
| The presence of the [`boundSocket.isPipe`][] getter on | ||
| `net.BoundSocket.prototype` is a capability signal that a build honors the | ||
| `path` option rather than silently binding a TCP ephemeral port. |
There was a problem hiding this comment.
I find the wording a bit confusing here; "a build" makes me think this is some like platform configuration option, not a Node.js version thing.
Better wording could be: "on Node.js versions where BoundSocket exists but predates path support, the option is silently ignored and a TCP ephemeral port is bound; the presence of isPipe on the prototype distinguishes versions that support path."
| // rather than inferring pipe-ness from a path option on the connect call. | ||
| // Other pre-existing handles (e.g. a TLSWrap) are not transport handles, so | ||
| // fall back to the path option in that case. | ||
| const pipe = this[kBoundSource] ? this._handle instanceof Pipe : !!path; |
There was a problem hiding this comment.
What happens here if this._handle is null from the if (this.destroyed) { line just before?
3 participants
Extends
net.BoundSocket(#63951) with apathoption so it can immediately bind a named unix-domain socket (or Windows pipe) synchronously, not just TCP. This gives a public synchronous UDS bind through the same role-neutral handle, instead of reaching intoprocess.binding('pipe_wrap'), just as it replaces the need fortcp_wrapandudp_wrapsimilarly.pathis mutually exclusive withhost/port/ipv6Only/reusePort, and binds in the constructor so conflicts throw there like TCP does. A leading'\0'uses the Linux abstract namespace; an abstract path elsewhere throwsERR_INVALID_ARG_VALUE.address()returns the path string for a pipe (asServer.address()does), the address object for TCP.isPipegetter to tell the two apart.server.listen(bound)andnew net.Socket({ handle: bound }).connect()pick pipe vs TCP from the adopted handle's type.localAddress.Note two error codes match libuv rather than intuition: missing parent dir is
EACCES(libuv mapsENOENT), over-long path isEINVAL(UV_PIPE_NO_TRUNCATE).Tests cover sync bind +
address(), duplicate-bindEADDRINUSE, the listen/connect round-trip, bound-clientlocalAddress, the abstract namespace, and the error paths.